feat(server): check optional bearer access token#382
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
FilOzzy
added
team/filecoin-pin
There was a problem hiding this comment.
Pull request overview
Adds an optional server-side bearer access token check to provide minimal request authentication for the Filecoin pinning API server when configured.
Changes:
- Add
Config.accessTokenand load it fromACCESS_TOKEN. - Add
--access-tokenCLI flag that setsACCESS_TOKEN. - Enforce configured token matching in the server
preHandlerhook.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/filecoin-pinning-server.ts | Rejects requests whose bearer token doesn’t match config.accessToken when configured. |
| src/core/synapse/index.ts | Extends shared Config type with accessToken. |
| src/config.ts | Loads accessToken from process.env.ACCESS_TOKEN. |
| src/commands/server.ts | Adds CLI option to set ACCESS_TOKEN via --access-token. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/filecoin-pinning-server.ts
Outdated
| if (config.accessToken && token !== config.accessToken) { | ||
| await reply.code(401).send({ error: 'Invalid access token' }) | ||
| return | ||
| } |
There was a problem hiding this comment.
The new config.accessToken enforcement branch isn’t covered by tests right now. Please add coverage that verifies requests are rejected with 401 when a configured access token does not match, and accepted when it matches (and consider a test for missing/invalid header behavior when the access token is configured).
There was a problem hiding this comment.
There is some server test coverage on another branch (#376) that I would prefer to extend, instead of adding another test here. Fewer merge conflicts that way.
| // Synapse SDK configuration | ||
| privateKey: process.env.PRIVATE_KEY, // Required: Ethereum-compatible private key | ||
| accessToken: process.env.ACCESS_TOKEN, | ||
| rpcUrl, // Determined from RPC_URL, NETWORK, or default to calibration |
There was a problem hiding this comment.
createConfig() now reads ACCESS_TOKEN, but the surrounding configuration docs only mention PRIVATE_KEY, RPC_URL, and NETWORK. Consider updating the comment/docs to include ACCESS_TOKEN (and what format is expected: the raw token value, not the Bearer prefix).
There was a problem hiding this comment.
I think this is obvious, but I've also done a lot of stuff with bearer tokens.
There was a problem hiding this comment.
You should move accessToken up to the "Application-specific" section cause this section is documented as the Synapse fields
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| // If no access token is configured, allow all requests | ||
| if (!config.accessToken) { | ||
| request.user = DEFAULT_USER_INFO | ||
| return | ||
| } |
There was a problem hiding this comment.
@rvagg might have better insight, but this seems okay to me. may be easy to miss for consumers though
There was a problem hiding this comment.
I think this is good because you will often be running this pinning server on the same machine as the client (e.g. pinmfs). If we don't want this behavior, then we need to make access-token mandatory.
github-project-automation
bot
moved this from 🔎 Awaiting review
to ✔️ Approved by reviewer
in FOC
rvagg
left a comment
rvagg
left a comment
There was a problem hiding this comment.
I don't even remember writing this code! So long ago. Maybe I was stubbing future functionality.
Seems fine to me, just move that field in config.ts.
|
@wjmelements : will let you make the update and merge - thanks. |
Reviewer @SgtPooki
This authentication provides minimal security for servers that need it.
If access token is supplied, reject requests not matching access token
Changes