fix(api): refuse structured run parameters at the boundary (ADR D72) - #265
Merged
Conversation
Array/object parameter values were documented out of scope for v1, but nothing rejected
them, and what happened next depended on which backend ran the job: the sync and
in-memory paths handed the source a JsonElement — the very type an ADO provider cannot
bind — while Hangfire round-tripped the bag and handed over raw JSON text. Either way
the caller learned about the limit as a driver error partway through a run, attributed
to the source rather than to the request that caused it.
POST /reports/{name}/run now answers 400 naming the offending parameter, so the
documented limit is real and identical on every backend, and the failure lands where the
caller can act on it. Scalars are untouched — null explicitly still binds, since an
optional parameter is an ordinary thing to send.
Deliberately not applied to source property bags, which travel the same object?-valued
shape: those are a provider's own configuration surface rather than a value bound into a
query, and nothing in the audit showed them failing this way.
Five tests, three of them verified to fail against the unfixed code. Both run modes are
covered because the divergence between them was the defect, and the null case pins that
the guard did not over-reach.
This closes the last item of docs/STATUS-AND-BACKLOG.md §6 that did not need a
maintainer decision.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…eries they are CodeQL flagged two loops that filter their sequence implicitly (cs/linq/missed-where, alerts 293 and 294). Both are mine: 294 is this PR's parameter check, and 293 is the Link-header loop on master — #264 cleared cs/linq/missed-select there and CodeQL immediately raised missed-where on the same loop, so the guard-on-the-way-in shape traded one alert for another rather than fixing the cause. Splitting the parse of a single RFC 8288 link-value into its own method leaves ParseLinkHeaderNext reading as what it actually is — the first rel="next" target, if any — and removes the whole class of finding instead of moving it. The parameter check becomes the FirstOrDefault it always was; a default KeyValuePair has a null Key, which is precisely the "no complex parameter" answer. Behaviour is unchanged: Http 60/60, AspNetCore 159/159. The explanatory comment moved from between `=>` and the expression into the XML doc — dotnet format rejects a comment in that position, and it belongs with the rest of the method's documentation anyway.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last item of
docs/STATUS-AND-BACKLOG.md§6 that did not need a maintainer decision.The defect
Array/object parameter values were documented out of scope for v1, but nothing rejected them — and what happened next depended on which backend ran the job:
JsonElement— the very type an ADO provider cannot bindJobParametersEither way the caller learned about the limit as a driver error partway through a run, attributed to the source rather than to the request that caused it.
The fix
POST /reports/{name}/runanswers 400 naming the offending parameter. That makes the documented limit real and identical on every backend, and moves the failure to the moment the caller can act on it.Scalars are untouched.
nullexplicitly still binds — an optional parameter is an ordinary thing to send, and there's a test pinning that the guard didn't over-reach into rejecting it.The check is deliberately not applied to source property bags, which travel the same
object?-valued shape and go through the same normalizer. Those are a provider's own configuration surface rather than a value bound into a query, and nothing in the audit showed them failing this way — so widening the guard there would be speculation, not a fix.Tests
Five added; three verified to fail against the unfixed code:
[Theory]over both run modes — the divergence between them was the defect, so covering only one would miss half of it.nullstill accepted.Each rejection test also asserts the source captured nothing, i.e. the request was refused before anything ran.
AspNetCore.IntegrationTests: 159/159, 0 failures.With this merged, §6's remaining entries are only the ones I flagged as needing your call — chiefly the⚠️ S3 key template not guarding caller-controlled parameters, which is breaking to fix for anyone passing a hierarchy fragment as a run parameter.