fix(files): authenticate private downloads with personal API keys - #301
Conversation
Preserve OAuth bearer authentication and omit credentials for signed URLs. Closes linearis-oss#300
iamfj
left a comment
There was a problem hiding this comment.
Thanks for this, Jacek. Issue #300 is one of the better bug reports this repo has had: a reproduction that narrows it to a single header, a permalink to the exact lines, and no credential or private asset URL pasted anywhere. That saved me the entire diagnosis.
I reproduced your verification at 895588c. Reverting the src change alone:
$ npx vitest run tests/unit/services/file-service.test.ts
Test Files 1 failed (1)
Tests 1 failed | 9 passed (10)
Only the personal-key row fails, which is the right shape for a regression test. With the fix back in:
$ npm test
Test Files 79 passed (79)
Tests 1211 passed (1211)
Same numbers you reported.
One change before I merge, and it is a subtraction. Full reasoning inline on file-service.ts; the short version is that linearis has no OAuth flow, both other credential-sending call sites already forward the token verbatim, and Linear documents no format guarantee for OAuth access tokens for the prefix check to lean on. headers["Authorization"] = this.apiToken; fixes #300 and matches graphql-client.ts:83.
Push the simplification and I will merge.
|
Makes sense, I'll drop the OAuth branch and keep the tests focused on raw API-key auth and signed URLs. |
iamfj
left a comment
There was a problem hiding this comment.
That is the change, thank you. fc32707 drops to one line and the download path now reads the same as graphql-client.ts:83 and the upload mutation twenty lines below it. Three call sites, one rule.
The TEST_TOKEN change to a prefixless test_token was not in my review and it is the best part of the commit. I checked it against both earlier versions of the source:
# against origin/next, the original Bearer bug
Tests 1 failed | 7 passed (8)
# against 895588c, the prefix-sniffing version
Tests 1 failed | 7 passed (8)
- "Authorization": "test_token",
+ "Authorization": "Bearer test_token",
A fixture carrying the lin_api_ prefix would have passed the second one. Yours does not, so credential-format inference cannot come back into this file without a red test. That is the kind of test I would rather have than three more rows.
Full suite on your head commit:
$ npm test
Test Files 79 passed (79)
Tests 1209 passed (1209)
npx tsc --noEmit reports the same 50 errors as origin/next, none of them in the files you touched. I could not run npm run knip locally, so CI will have to cover that one.
Approving. Thanks for the patience through a review that asked you to delete most of your first fix, and for taking the reasoning seriously rather than just applying the diff.
What happens next
Merging this into next cuts a prerelease immediately, published to npm under the next dist-tag:
npm i -g linearis@next
Once the release job goes green, grab it and point it at the private asset from #300. A pass in your own environment is worth more to me than my test run, since neither of us can assert what uploads.linear.app does from a mocked fetch.
Stable is further out. I want #302 merged and validated first, probably batched with some of the pending Renovate upgrades, before any of this promotes to main and the latest tag. Nothing for you to do, just so you know why it will sit on next for a while.
✅ knip — no dead codeNo unused files, exports, types, or dependencies detected. |
fc32707 to
5c720e7
Compare
|
Heads up: I pushed a commit message fix to your branch, since The body of New head is You will want to |
Remove token-prefix inference and match the GraphQL client's authorization header. Keep explicit raw-header and signed-URL test cases, using a prefixless fixture to guard against credential-format assumptions. Refs linearis-oss#300
5c720e7 to
fa86ebb
Compare
What does this PR do?
lin_api_) as raw Authorization values for private downloads instead of prefixing them with Bearer.Closes #300
Type of change
Checklist
npm run check:cipasses (lint + format)npx tsc --noEmitpasses (type check)npm testpasses (unit tests)Testing
Local verification at
895588c94377c4f1cdcba9134448eed8a54e622d:Bearer lin_api_test_token.npx vitest run tests/unit/services/file-service.test.tspassed all 10 tests, including existing failure-path coverage.npm test: 1,211 tests passed across 79 files.npm run check:ci,npx tsc --noEmit,npm run build, andnpm run knipexited successfully. Biome reports an existing informational schema-version mismatch (configuration 2.5.6, CLI 2.5.8).These are local checks, not a claim of hosted CI or live OAuth verification.
Notes for reviewers
The service receives a token string without credential-type metadata. The fix uses the
lin_api_prefix to recognize personal API keys and preserves the existing Bearer behavior for other tokens, avoiding a new configuration option or changes to token storage. Signed-URL handling remains unchanged and bypasses Authorization for either credential type.The reproduction in #300 used the same personal key and private asset: Bearer authorization returned 401, while raw authorization returned 200 with a valid ZIP signature. OAuth compatibility is covered by request-header assertions, not a live OAuth download.