fix(dart): send project ID as a header on location methods - #1732
Open
ChiragAgg5k wants to merge 5 commits into
Open
fix(dart): send project ID as a header on location methods#1732ChiragAgg5k wants to merge 5 commits into
ChiragAgg5k wants to merge 5 commits into
Conversation
Location methods (getFileDownload, getFilePreview, getFileView, avatars.*) built their auth from the legacy `method.auth` loop, which put project into the query string and sent no headers. The Project security scheme is non-global, so `setProject()` only populates `client.config['project']` — the `X-Appwrite-Project` header is attached per request by the other request templates. Location methods therefore reached the server with an API key but no project header, and the server's API key check reads that header specifically, failing with `project_id_missing (403)`. Build headers from `method.securityHeaders` + `method.headers`, matching api.twig. Also fixes the missing `accept: */*` header on these methods. OAuth/webAuth methods keep project in the query string — those are browser redirect URLs where headers aren't possible.
Contributor
Greptile SummaryThe PR updates generated location requests to send project authentication through
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "test: set a project on the .NET e2e clie..." | Re-trigger Greptile |
The bug shipped because nothing asserted it. Two layers: Unit (tests/unit/LocationMethodTest.php) renders the Dart and Flutter SDKs from the fixture spec and asserts general.download() carries X-Appwrite-Project as a header and not as a query parameter. Runs in the Unit suite, no Docker, and fails against the pre-fix templates. E2E closes the blind spot that let this through: the mock server accepted the download request without any project header, and neither Dart nor Flutter exercised a location method at all. The download route now requires the header, mirroring /v1/ping, and both scripts call general.download(). Go never set a project on its client, so it only passed the new assertion by accident of not being checked before — it now sets one like every other language.
The e2e coverage added alongside it exercises the same path against a real request, so asserting on the rendered template text was redundant.
Extending the e2e download assertion to all SDKs surfaced the same bug in five more targets. Location methods must send X-Appwrite-Project as a header: the Project security scheme is non-global, so setProject() only populates client config, and the server pairs an API key against that header specifically. - deno: only multipart methods sent the header — every other method, location or not, omitted it. Now built from securityHeaders like the upload branch already did. - kotlin, swift: apiHeaders was built but never passed to client.call() for location methods. - android, apple: location methods pushed project into the query string via the legacy method.auth loop and passed no headers. Apple's headers map also had to move above the branch so the shared location template can see it. webAuth keeps project in the query string everywhere — those are browser redirect URLs where headers aren't possible. Every SDK's e2e script now calls general.download(), and the mock server requires the header on that route. php, python, ruby and deno never set a project on their client, so they now do. Web is excluded: its location methods return a URL string and issue no request. CLI is excluded: its location commands write to a --destination file in a separate harness, and it inherits Node's already-correct client.
The .NET SDK sends X-Appwrite-Project on location methods correctly, but its e2e script never set a project, so the header went out empty and the mock server's new assertion rejected the download. Same gap already fixed for go, php, python, ruby and deno.
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.
What does this PR do?
Fixes
project_id_missing (403)onlocation-type methods —storage.getFileDownload(),getFilePreview(),getFileView(), and allavatars.*— when authenticating with a project API key. Seven SDKs were affected.The
Projectsecurity scheme is non-global, sosetProject()only populatesclient.config; attachingX-Appwrite-Projectis the per-request template's job. The server's API key check reads that header specifically, so a request carrying a valid key but no project header is rejected regardless of scopes.Before
After
Side benefit: these methods were also missing the
accept: */*header.Affected SDKs
method.authloop, no headers passedapiHeaderswas built but never passed to the location callmethod.auth, no headers. Apple's headers map also had to move above the branch so the shared location template can see itGo, .NET, Node, PHP, Python, Ruby and React Native were already correct. Rust and Unity attach the header at client level in
set_project/SetProject, so they were never affected.webAuthkeepsprojectin the query string everywhere — those are browser redirect URLs where headers aren't possible.The dropped
impersonateuseridline is not a regression.ImpersonateUserIdis a global scheme, so its setter already callsaddHeader(...)and the client merges it into every request — which is whysecurityHeadersdeliberately excludes globals. The old line was dead anyway: it readconfig['impersonateuserid'](fromheader|caseLower) while the setter writesconfig['impersonateUserId'], so it always resolved tonulland was dropped from GET params.projectonly worked there because it is spelled identically in both casings.Test Plan
The bug shipped because nothing asserted it: the mock server accepted the download request without any project header, and most SDKs never exercised a location method. Only Go, Rust and Unity asserted
DOWNLOAD_RESPONSES.x-appwrite-project, mirroring/v1/ping. This makes the bug detectable for every language target.general.download(), withDOWNLOAD_RESPONSESadded to the matching expectation lists.Failed asserting that null matches expected 'GET:/v1/mock/tests/general/download:passed', and .NET fails without its client-sideSetProjectand passes with it.composer lint,composer refactor:checkand the Unit suite pass. djlint reports the same 44 pre-existing errors before and after, none in the touched templates.Two deliberate exclusions: Web, whose location methods return a URL string and issue no request, so there is no header to assert; and CLI, whose location commands write to a
--destinationfile in a separate harness and which inherits Node's already-correct client.Related PRs and Issues
Reported on a self-hosted 1.9.5 instance calling
getFileDownloadfrom a Dart 3.12 function withdart_appwrite26.1.0. Workaround until the next SDK release:Separate finding, not addressed here: Ruby's location methods return a raw
Net::HTTPOKobject rather than bytes, unlike every other SDK. The e2e script works around it with.body.Have you read the Contributing Guidelines on issues?
Yes.