Skip to content

security: rate limit expensive routes, stop upload path traversal - #159

Closed
NetworkTheoryAppliedResearchInstitute wants to merge 2 commits into
mainfrom
security/codeql-rate-limiting-and-path-traversal
Closed

NetworkTheoryAppliedResearchInstitute wants to merge 2 commits into
mainfrom
security/codeql-rate-limiting-and-path-traversal

Conversation

@NetworkTheoryAppliedResearchInstitute

Copy link
Copy Markdown
Collaborator

Clears the nine open CodeQL alerts on main that #148 does not already cover. With #148 merged, Agrinet's code-scanning queue goes to zero.

What was wrong

js/path-injection — backend/models/message.js:31. file.name comes from the client and was concatenated straight into the write path:

const filename = Date.now() + '_' + file.name;
fs.writeFileSync(path.join(UPLOAD_DIR, filename), ...);

A message with file.name = "../../etc/passwd" wrote outside UPLOAD_DIR. Now the name is collapsed with path.basename, stripped to [A-Za-z0-9._-] (which also removes NUL bytes and leading dots), and the write is refused outright if the resolved path is not directly inside UPLOAD_DIR.

Checked against photo.png, ../../etc/passwd, ..%2f..%2fetc, ....//....//x, a/b/c.png, ../.ssh/authorized_keys, \0evil.sh, "", null, .., ..... — every one stays inside the directory.

js/missing-rate-limiting — 8 alerts. No limiter existed anywhere. Adds express-rate-limit and backend/middleware/rateLimit.js with three ceilings per 15-minute window:

limiter cap applied to
apiLimiter 600 global in server.js, plus agrotourism reads
writeLimiter 60 logs, deposits, conversations, communication
uploadLimiter 20 POST /create (up to 5 images)

The global limiter sits ahead of authMiddleware so unauthenticated floods are shed before any auth work happens.

actions/missing-workflow-permissions. check-hardcoded-urls.yml now declares contents: read.

Notes for review

  • Limiters are applied at router level via .use(), not as an extra route argument. The tracked express stub registers routes as (path, handler) — inserting a third argument would have silently dropped the real handler.
  • express-rate-limit was added with --package-lock-only so the three tracked stubs under backend/node_modules are untouched. A matching stub joins them (force-added, since backend/.gitignore:26 ignores node_modules/) so npm test runs without a full install.

Verification

npm test in backend/: 6/6 pass, identical to the pre-patch baseline. node --check clean on all eight touched JS files.

🤖 Generated with Claude Code

Clears the nine open CodeQL alerts that PR #148 does not cover.

js/path-injection (backend/models/message.js:31): file.name arrives from
the client and was concatenated straight into the upload path, so a name
like '../../etc/passwd' wrote outside UPLOAD_DIR. Collapse the name with
path.basename, strip anything outside [A-Za-z0-9._-] (which also removes
NUL bytes and leading dots), and refuse the write if the resolved path is
not directly inside UPLOAD_DIR.

js/missing-rate-limiting (8 alerts): add express-rate-limit and a shared
backend/middleware/rateLimit.js exposing three ceilings per 15m window --
apiLimiter (600), writeLimiter (60) and uploadLimiter (20). Applied at
router level via .use() rather than as an extra route argument, so the
existing two-argument route signatures are untouched. server.js takes the
global apiLimiter ahead of authMiddleware so unauthenticated floods are
shed before auth work happens.

actions/missing-workflow-permissions: check-hardcoded-urls.yml now
declares contents: read.

express-rate-limit was added with --package-lock-only so the three tracked
stub files under backend/node_modules are left intact, and a matching
express-rate-limit stub joins them so `npm test` still runs without a full
install.

backend suite: 6/6 pass, unchanged from before the patch.

Signed-off-by: Jodson Graves <info@ntari.org>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
node_modules is gitignored, so the stub added alongside rateLimit.js was
silently dropped from the previous commit while the three existing stubs
(express, aws-sdk, stripe) stayed tracked because they were force-added.
Force-add this one the same way, so `npm test` resolves the module without
a full install.

Signed-off-by: Jodson Graves <info@ntari.org>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@NetworkTheoryAppliedResearchInstitute

Copy link
Copy Markdown
Collaborator Author

Superseded by #161: same changes, rebased onto current main and squashed into one commit whose Signed-off-by matches the commit author, which fixes the DCO failure here.

@NetworkTheoryAppliedResearchInstitute
NetworkTheoryAppliedResearchInstitute deleted the security/codeql-rate-limiting-and-path-traversal branch September 14, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant