Repository navigation
Never send resource, license, licence or values from Evidence to the cloud - #61
Merged
Merged
Conversation
jwrosewell
force-pushed
the
evidence-reserved-keys
branch
from
November 9, 2026 23:52
f21a356 to
4293d31
Compare
This was referenced Nov 10, 2026
…es test rc now sends the resource key and license key in the request body, set after the Evidence, so the test expects those two fields with the engine's values, no license in the URL, and no other field whose name is a reserved one.
jwrosewell
marked this pull request as ready for review
November 11, 2026 13:51
This was referenced Nov 11, 2026
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
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 Cloud Request Engine now leaves out of its request any Evidence whose name after the prefix is
resource,license,licenceorvalues, in any letter case and under any prefix, as the Processing section of the Cloud Request Engine page says.Problem
addQueryDatainpipeline/fiftyone.pipeline.cloudrequestengine/cloudRequestEngine.jscopied every piece of Evidence into the request body under its name after the prefix, whatever the name. Onrcthe engine sets its ownresource,licenseandvaluesfields after the Evidence, so Evidence of exactly those names in lower case is replaced. Evidence such asquery.LICENSEorheader.Resourcestill became a second field of the same name in other letters,licencewas sent as it was, andvalueswas sent whenever the engine had no property list of its own.What the specification says
The Processing section of cloud-request-engine.md at e89679e9, the specifications
rchead:and
Change
addQueryDataskips Evidence whose name after the prefix is one of the four names in thereservedFieldsconstant, compared without regard to case. A key with no dot is its own name, so it is covered too, and the test pins that with a bareLicencekey. The check runs before the precedence logic, so a reserved name never claims a field.rcalready does.evidencekeysanswer, so it holds for every piece of Evidence the engine sends.Tests
evidence never supplies resource, license or valuesinpipeline/fiftyone.pipeline.cloudrequestengine/tests/cloudRequestEngine.test.jsaddsquery.Resource,query.LICENSE,query.Licence,query.Values,header.resourceandcookie.License, plusquery.other,header.otherandcookie.cookie-only, and a bareLicencewith no prefix, added in3160539. It captures the POST and checks that the URL carries the resource key and no license, that the only reserved fields in the body areresourceandlicenseholding the engine's own keys, that none of the Evidence values for the reserved names appear, and that the other Evidence is sent withquery.still taking precedence.35b2e3dupdates the test afterrcwas merged in ate1a4c6c, becausercnow sends the keys in the body rather than the license key in the URL.Every temporary run below sits in one workflow file shared by the temporary runs, so its page can carry the name of another branch in its title. Each run's own branch is named here.
rcwithout the change: run 42721446746. Green: run 42721462583, 22 passed.rcatc32a2905with this draft's test file, on branchtmp/node-61-red: run 43001275649. The new test fails and the other 21 pass.0253418, on branchtmp/node-61-green: run 43001293870, 22 of 22 pass.0253418, success, run 43001376753. Configure and the Ubuntu, Ubuntu ARM and Windows jobs on Node 22 and 24 pass, 998 of 1000 tests with 2 skipped in each, Configure printed "Left out 2 macOS entries", and all 7Setting 'PullRequestSha'lines are0253418f9d984e715570aca53cc95c009cbf0aa6.tmp/node-61-red3, run 43024570384, the test at3160539onrccode. The test fails and the other 21 pass.3160539, success, run 43024622299. Configure and the Ubuntu, Ubuntu ARM and Windows jobs on Node 22 and 24 pass, 1152 of 1154 tests with 2 skipped in each, Configure printed "Left out 2 macOS entries", and all 7Setting 'PullRequestSha'lines are316053942bb25efcdd6368848cbcd8cc814735c7.macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
What remains
isSent. This draft merges first, and Send the cloud every Evidence value apart from the reserved names #72 then drops its own check in favour of this one and keeps its check for the web request view underserver.http-request.