Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: prevent non-string properties from being passed as context values (
#6676) This change fixes the OpenAPI schema to disallow non-string properties on the top level of the context (except, of course, the `properties` object). This means that we'll no longer be seeing issues with rendering invalid contexts, because we don't accept them in the first place. This solution comes with some tradeoffs discussed in the [PR](#6676). Following on from that, this solution isn't optimal, but it's a good stop gap. A better solution (proposed in the PR discussion) has been added as an idea for future projects. The bulk of the discussion around the solution is included here for reference: @kwasniew: Was it possible to pass non string properties with our UI before? Is there a chance that something will break after this change? @thomasheartman: Good question and good looking out 😄 You **could** pass non-string, top-level properties into the API before. In other words, this would be allowed: ```js { appName: "my-app", nested: { object: "accepted" } } ``` But notably, non-string values under `properties` would **not** be accepted: ```js { appName: "my-app", properties: { nested: { object: "not accepted" } } } ``` **However**, the values would not contribute to the evaluation of any constraints (because their type is invalid), so they would effectively be ignored. Now, however, you'll instead get a 400 saying that the "nested" value must be a string. I would consider this a bug fix because: - if you sent a nested object before, it was most likely an oversight - if you sent the nested object on purpose, expecting it to work, you would be perplexed as to why it didn't work, as the API accepted it happily Furthermore, the UI will also tell you that the property must be a string now if you try to do it from the UI. On the other hand, this does mean that while you could send absolute garbage in before and we would just ignore it, we don't do that anymore. This does go against how we allow you to send anything for pretty much all other objects in our API. However, the SDK context is special. Arbitrary keys aren't ignored, they're actually part of the context itself and as such should have a valid value. So if anything breaks, I think it breaks in a way that tells you why something wasn't working before. However, I'd love to hear your take on it and we can re-evaluate whether this is the right fix, if you think it isn't. @kwasniew: Coming from the https://en.wikipedia.org/wiki/Robustness_principle mindset I'm thinking if ignoring the fields that are incorrect wouldn't be a better option. So we'd accept incorrect value and drop it instead of: * failing with client error (as this PR) or * saving incorrect value (as previous code we had) @thomasheartman: Yeah, I considered that too. In fact, that was my initial idea (for the reason you stated). However, there's a couple tradeoffs here (as always): 1. If we just ignore those values, the end user doesn't know what's happened unless they go and dig through the responses. And even then, they don't necessarily know why the value is gone. 2. As mentioned, for the context, arbitrary keys can't be ignored, because we use them to build the context. In other words, they're actually invalid input. Now, I agree that you should be liberal in what you accept and try to handle things gracefully, but that means you need to have a sensible default to fall back to. Or, to quote the Wikipedia article (selectively; with added emphasis): > programs that receive messages should accept non-conformant input **as long as the meaning is clear**. In this case, the meaning isn't clear when you send extra context values that aren't strings. For instance, what's the meaning here: ```js { appName: "my-app", nested: { object: "accepted", more: { further: "nesting" } } } ``` If you were trying to use the `nested` value as an object, then that won't work. Ideally, you should be alerted. Should we "unwind" the object and add all string keys as context values? That doesn't sound very feasible **or** necessarily like the right thing. Did you just intend to use the `appName` and for the `nested` object to be ignored? And it's because of this caveat that I'm not convinced just ignoring the keys are the right thing to do. Because if you do, the user never knows they were ignored or why. ---- **However**, I'd be in favor of ignoring they keys if we could **also** give the users warnings at the same time. (Something like what we do in the CR api, right? Success with warnings?) If we can tell the user that "we ignored the `a`, `b`, and `c` keys in the context you sent because they are invalid values. Here is the result of the evaluation without taking those keys into account: [...]", then I think that's the ideal solution. But of course, the tradeoff is that that increases the complexity of the API and the complexity of the task. It also requires UI adjustments etc. This means that it's not a simple fix anymore, but more of a mini-project. But, in the spirit of the playground, I think it would be a worthwhile thing to do because it helps people learn and understand how Unleash works.
- Loading branch information