Skip to content

fix: Disallow subscribing to values of the wrong kind #6934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 15, 2025

Conversation

jkeiser
Copy link
Contributor

@jkeiser jkeiser commented Aug 14, 2025

Right now, PUT /attributes lets you subscribe to a value of the wrong kind. Since Claude is doing a lot of PUT /attributes, we need that not to work (because sometimes it makes mistakes, and some of those mistakes can mess up the workspace, including subscribing from a non-secret to a secret.)

array -> string / array -> object subscriptions are disallowed the same way; as such, this PR also removes the long-deprecated automatic si:normalizeToArray function in subscriptions (since now all you can do is array-to-array). There is no way in the UI to subscribe from an array to a single value, so this should not cause problems in the UI. We have left the keepExistingSubscriptions parameter in the API until we're sure clients have transitioned, but it's ignored and is no longer in tests or docs.

Out of Scope:

  • We should also check if a secret prop is subscribing to a non-secret, even if they are the same prop kind.
  • The UI doesn't check the target prop's type right now, and lets you try to subscribe; you get a silent error if you do it wrong. This isn't much different from before, where the subscription succeeded but the value was either empty or wrong!

How was it tested?

  • Integration tests pass
  • New integration tests for subscribing to the wrong type
  • Manual test: subscribing array -> array, map -> map, object -> object, string -> string, number -> number works
  • AI test: When you ask Claude to subscribe to a secret using /resource_value, it gets a reasonable error back and then corrects itself.

In short: 🔗

@jkeiser jkeiser requested a review from vbustamante August 14, 2025 00:50
@github-actions github-actions bot added A-sdf Area: Primary backend API service [Rust] A-docs Area: Project documentation A-dal A-web A-dal-test A-luminork labels Aug 14, 2025
Copy link

github-actions bot commented Aug 14, 2025

Dependency Review

✅ No vulnerabilities or OpenSSF Scorecard issues found.

Scanned Files

None

@jkeiser jkeiser force-pushed the jkeiser/subscription-type-mismatch branch from f06b572 to dd1b378 Compare August 14, 2025 00:59
let maybe_func_id = if let Some(func) = sub.function.clone() {
func.resolve(ctx).await?
} else {
None
};

AttributeValue::set_to_subscriptions(
AttributeValue::set_to_subscription(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be an example where we want to turn the error into a more friendly response that returns from luminork vs. the 500 that it'll probably return now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, will the 500 omit the actual painstakingly crafted error message?

britmyerss
britmyerss previously approved these changes Aug 14, 2025
@stack72 stack72 force-pushed the jkeiser/subscription-type-mismatch branch from dd1b378 to 88de262 Compare August 15, 2025 17:51
@stack72 stack72 added this pull request to the merge queue Aug 15, 2025
Merged via the queue into main with commit 9660d53 Aug 15, 2025
11 checks passed
@stack72 stack72 deleted the jkeiser/subscription-type-mismatch branch August 15, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dal A-dal-test A-docs Area: Project documentation A-luminork A-sdf Area: Primary backend API service [Rust] A-web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants