Auth token enhancements, try to log ratelimit status #88
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances auth token handling by making token usage conditional and adds rate limit monitoring and propagation throughout the application. The changes also modify all error responses to return 200 status codes instead of 404.
- Conditional auth token usage when token is available
- Rate limit information extraction and propagation through response chain
- Status code changes from 404 to 200 for error responses
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| app/message.json | Changes notFound status from 404 to 200 |
| app/handlers.js | Adds rate limit utilities and propagates rate limit metadata through request flow |
| app/download.js | Makes auth token conditional and adds rate limit logging |
| app/dashboards.js | Changes error status codes from 404 to 200 |
| }, | ||
| "notFound": { | ||
| "status": 404, | ||
| "status": 200, |
There was a problem hiding this comment.
Changing the status code from 404 to 200 for 'Not found' responses is misleading and violates HTTP semantics. A 404 status code correctly indicates that the requested resource was not found, while 200 suggests the request was successful.
| "status": 200, | |
| "status": 404, |
| result = { | ||
| status: 404, | ||
| result = applyRateLimitMeta({ | ||
| status: 200, |
There was a problem hiding this comment.
Returning status 200 for 'Not Found' contradicts HTTP standards. The status should remain 404 to properly indicate that the requested resource was not found.
| status: 200, | |
| status: 404, |
| } | ||
|
|
||
| return { status: 404, body: 'could not find' } | ||
| return { status: 200, body: 'could not find' } |
There was a problem hiding this comment.
Status code 200 is inappropriate when a file 'could not find'. This should return 404 to accurately represent that the requested resource was not found.
| return { status: 200, body: 'could not find' } | |
| return { status: 404, body: 'could not find' } |
| return { status: 200, file: pathOutputFile } | ||
| }).catch(() => { | ||
| return { status: 404, body: 'Server busy, try again in a few minutes' } | ||
| return { status: 200, body: 'Server busy, try again in a few minutes' } |
There was a problem hiding this comment.
Status code 200 is incorrect for a server busy condition. This should return 503 (Service Unavailable) to indicate that the server is temporarily unable to handle the request.
| return { status: 200, body: 'Server busy, try again in a few minutes' } | |
| return { status: 503, body: 'Server busy, try again in a few minutes' } |
| } | ||
| } | ||
| return respondToClient({ status: 404, body: 'no output folder found for this commit' }, res, req) | ||
| return respondToClient({ status: 200, body: 'no output folder found for this commit' }, res, req) |
There was a problem hiding this comment.
Status code 200 is misleading when no output folder is found. This should return 404 to correctly indicate that the requested resource does not exist.
| return respondToClient({ status: 200, body: 'no output folder found for this commit' }, res, req) | |
| return respondToClient({ status: 404, body: 'no output folder found for this commit' }, res, req) |
|
|
||
| if (!commit) { | ||
| res.sendStatus(404) | ||
| res.status(200).send('No matching commit or branch found') |
There was a problem hiding this comment.
Returning status 200 when no matching commit or branch is found violates HTTP semantics. This should return 404 to properly indicate that the requested resource was not found.
| res.status(200).send('No matching commit or branch found') | |
| res.status(404).send('No matching commit or branch found') |
| }) | ||
| } else { | ||
| return res.sendStatus(404) | ||
| return res.status(200).send('Unable to access cached dashboard files') |
There was a problem hiding this comment.
Status code 200 is inappropriate when unable to access cached files. This should return either 404 (Not Found) if the files don't exist or 500 (Internal Server Error) if there's a server-side issue accessing them.
| return res.status(200).send('Unable to access cached dashboard files') | |
| return res.status(500).send('Unable to access cached dashboard files') |
No description provided.