fix: Improve application submission and stability#245
Conversation
- Add `TRUSTED_PROXY` environment variable to configure client IP resolution for rate limiting and add documentation for common setups. - Add logging for rate limit failures to help diagnose IP resolution issues. - Improve job application submission reliability by clamping attribution tracking parameters instead of failing validation. - Implement field-level validation errors for public application forms to provide actionable feedback to users. - Prevent published job slugs from changing on title edits to avoid breaking existing public links. - Add character length constraints to applicant name fields.
|
Caution Review failedFailed to post review comments. We encountered an issue with GitHub. Use ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used🪛 OpenGrep (1.25.0)server/utils/rateLimit.ts[ERROR] 317-317: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead. (coderabbit.command-injection.exec-js) 📝 WalkthroughWalkthroughThe PR adds configurable trusted-proxy handling for rate limiting, structured public application validation errors, safer attribution parsing, conditional job slug updates, client-side input limits, documentation, and unit-test coverage. ChangesProxy-aware rate limiting
Public application validation
Conditional job slug updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ApplicationEndpoint
participant parsePublicApplication
participant ApplicationForm
Client->>ApplicationEndpoint: submit multipart or JSON application
ApplicationEndpoint->>parsePublicApplication: validate normalized input
parsePublicApplication-->>ApplicationEndpoint: parsed data or structured 400 error
ApplicationEndpoint-->>ApplicationForm: return success or field error
ApplicationForm->>ApplicationForm: display inline validation message
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JoachimLK
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
server/utils/env.ts (1)
89-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider validating
TRUSTED_PROXYvalues in the schema.The field accepts any string, so
TRUSTED_PROXY=trueorTRUSTED_PROXY=nginxpasses startup validation and only surfaces later as a runtimerate_limit.trusted_proxy_invalidlog while silently keeping the productioncloudflaredefault — the exact misconfiguration this PR documents as causing spurious 429s. A schema-level constraint turns it into a boot-time failure.♻️ Proposed schema constraint
- TRUSTED_PROXY: emptyToUndefined.optional(), + TRUSTED_PROXY: emptyToUndefined + .pipe( + z.string().regex( + /^(cloudflare|none|[1-9]|10)$/i, + 'TRUSTED_PROXY must be "cloudflare", "none", or a hop count 1-10', + ), + ) + .optional(),Note that
server/utils/rateLimit.tsintentionally readsprocess.envdirectly, so this only adds a startup guard; it does not change resolution behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/utils/env.ts` around lines 89 - 98, Update the TRUSTED_PROXY schema in the environment configuration to accept only the documented values: cloudflare, none, or a numeric hop count from 1 through 10. Preserve the existing optional and empty-to-undefined behavior, while rejecting arbitrary strings such as true or nginx during startup; leave TRUSTED_PROXY_IP and rateLimit resolution unchanged.tests/unit/rate-limit.test.ts (2)
38-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the
NODE_ENVrestore against an originally-unset value.
process.env.NODE_ENV = undefinedstringifies to"undefined"rather than deleting the key, leaking a bogus value to other tests in the worker. Harmless for the=== 'production'checks here, but cheap to make correct.♻️ Proposed tweak
+function restoreNodeEnv() { + if (ORIGINAL_ENV.NODE_ENV === undefined) delete process.env.NODE_ENV + else process.env.NODE_ENV = ORIGINAL_ENV.NODE_ENV +} + beforeEach(() => { delete process.env.TRUSTED_PROXY delete process.env.TRUSTED_PROXY_IP - process.env.NODE_ENV = ORIGINAL_ENV.NODE_ENV + restoreNodeEnv() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/rate-limit.test.ts` around lines 38 - 45, Update the NODE_ENV restoration in beforeEach and afterEach to delete process.env.NODE_ENV when ORIGINAL_ENV.NODE_ENV was initially unset; otherwise restore its original value. Preserve restoration of an originally defined NODE_ENV and the existing proxy cleanup behavior.
218-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case where
X-Forwarded-Forhas fewer entries than the configured hop count.That's the branch where
forwardedIpFromRightclamps to index 0 and returns a client-written value — see the comment onserver/utils/rateLimit.tsLines 295-301. A test withTRUSTED_PROXY=2and a two-entry header where the leftmost is attacker-supplied would pin the intended behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/rate-limit.test.ts` around lines 218 - 225, Add a unit test alongside the existing X-Forwarded-For hop-count case that sets TRUSTED_PROXY to 2 and supplies a two-entry header with an attacker-controlled leftmost value, asserting getClientIp(makeEvent(...)) returns that leftmost value when forwardedIpFromRight clamps to index 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/pages/jobs/`[slug]/apply.vue:
- Around line 258-262: Update the field-level error mapping near the `field`
lookup so server responses naming `coverLetterText` are assigned to
`errors.coverLetter`, while retaining the existing `form.value` mapping for
other fields. Ensure the mapped cover-letter error renders inline rather than
remaining global.
In `@server/utils/rateLimit.ts`:
- Around line 191-205: Prevent invalid explicit TRUSTED_PROXY values from
falling through to the production Cloudflare default. In
server/utils/rateLimit.ts lines 191-205, make the invalid-value path fatal after
logging rate_limit.trusted_proxy_invalid, or explicitly document the intentional
fail-open behavior; in server/utils/env.ts lines 89-98, constrain TRUSTED_PROXY
validation to cloudflare, none, or numeric hop counts 1-10 so invalid
configuration fails during startup.
- Around line 113-122: Update the rate-limit logging in the exceeded-limit path
around resolveClientKey to avoid emitting the raw client IP: apply the
repository’s established privacy-preserving hashing or truncation helper to ip
before assigning client_key, while preserving client_key_source and the
remaining diagnostic fields.
- Around line 295-301: Update forwardedIpFromRight to return undefined when hops
exceeds entries.length instead of clamping the index to zero. Only normalize and
return the selected forwarded entry when the configured hop count is available,
preserving the empty-entry behavior so callers fall back to the socket key and
warning.
In `@server/utils/slugify.ts`:
- Around line 53-54: Update the draft branch in generateJobSlug to regenerate
only when newTitle differs from currentTitle, preserving an existing custom slug
when the submitted title is unchanged. Add a regression test covering an
unchanged draft title without customSlug and verify the custom slug remains
intact.
---
Nitpick comments:
In `@server/utils/env.ts`:
- Around line 89-98: Update the TRUSTED_PROXY schema in the environment
configuration to accept only the documented values: cloudflare, none, or a
numeric hop count from 1 through 10. Preserve the existing optional and
empty-to-undefined behavior, while rejecting arbitrary strings such as true or
nginx during startup; leave TRUSTED_PROXY_IP and rateLimit resolution unchanged.
In `@tests/unit/rate-limit.test.ts`:
- Around line 38-45: Update the NODE_ENV restoration in beforeEach and afterEach
to delete process.env.NODE_ENV when ORIGINAL_ENV.NODE_ENV was initially unset;
otherwise restore its original value. Preserve restoration of an originally
defined NODE_ENV and the existing proxy cleanup behavior.
- Around line 218-225: Add a unit test alongside the existing X-Forwarded-For
hop-count case that sets TRUSTED_PROXY to 2 and supplies a two-entry header with
an attacker-controlled leftmost value, asserting getClientIp(makeEvent(...))
returns that leftmost value when forwardedIpFromRight clamps to index 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af161633-f8f1-4341-87fd-b049f3f54b3b
📒 Files selected for processing (13)
.env.exampleSELF-HOSTING.mdapp/components/ApplicationFormBody.vueapp/pages/jobs/[slug]/apply.vueserver/api/jobs/[id].patch.tsserver/api/public/jobs/[slug]/apply.post.tsserver/utils/env.tsserver/utils/rateLimit.tsserver/utils/schemas/publicApplication.tsserver/utils/slugify.tstests/unit/job-slug.test.tstests/unit/public-application-validation.test.tstests/unit/rate-limit.test.ts
| // Field-level validation failures name their field — show it inline too | ||
| const field = err.data?.data?.field | ||
| if (status === 400 && typeof field === 'string' && field in form.value) { | ||
| errors.value[field] = message | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Map coverLetterText to the displayed cover-letter error key.
The server returns field: "coverLetterText", but this branch only accepts keys in form; cover letter state is separate and local validation uses errors.coverLetter. Server-side cover-letter validation errors therefore remain global instead of rendering beside the field.
Proposed fix
- const field = err.data?.data?.field
- if (status === 400 && typeof field === 'string' && field in form.value) {
+ const serverField = err.data?.data?.field
+ const field = serverField === 'coverLetterText' ? 'coverLetter' : serverField
+ if (status === 400 && typeof field === 'string' && (field in form.value || field === 'coverLetter')) {
errors.value[field] = message
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Field-level validation failures name their field — show it inline too | |
| const field = err.data?.data?.field | |
| if (status === 400 && typeof field === 'string' && field in form.value) { | |
| errors.value[field] = message | |
| } | |
| // Field-level validation failures name their field — show it inline too | |
| const serverField = err.data?.data?.field | |
| const field = serverField === 'coverLetterText' ? 'coverLetter' : serverField | |
| if (status === 400 && typeof field === 'string' && (field in form.value || field === 'coverLetter')) { | |
| errors.value[field] = message | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/pages/jobs/`[slug]/apply.vue around lines 258 - 262, Update the
field-level error mapping near the `field` lookup so server responses naming
`coverLetterText` are assigned to `errors.coverLetter`, while retaining the
existing `form.value` mapping for other fields. Ensure the mapped cover-letter
error renders inline rather than remaining global.
| // Log the resolved bucket key: a limit that trips for unrelated clients is | ||
| // almost always an IP-resolution problem (see resolveClientKey), and that | ||
| // is invisible without knowing which key was counted. | ||
| logWarn('rate_limit.exceeded', { | ||
| client_key: ip, | ||
| client_key_source: source, | ||
| path: getRequestURL(event).pathname, | ||
| limit: maxRequests, | ||
| window_ms: windowMs, | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Client IP is now written to logs — confirm this fits your retention/redaction posture.
client_key is a raw client IP (personal data under GDPR). Rate-limit abuse diagnostics is a defensible legitimate interest, but this repo already ships GDPR retention machinery, so the log sink should have a bounded retention window — or hash/truncate the key here if logs are long-lived.
🛡️ Optional: log a truncated key instead
logWarn('rate_limit.exceeded', {
- client_key: ip,
+ client_key: maskIp(ip),
client_key_source: source,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/utils/rateLimit.ts` around lines 113 - 122, Update the rate-limit
logging in the exceeded-limit path around resolveClientKey to avoid emitting the
raw client IP: apply the repository’s established privacy-preserving hashing or
truncation helper to ip before assigning client_key, while preserving
client_key_source and the remaining diagnostic fields.
| logWarn('rate_limit.trusted_proxy_invalid', { | ||
| value: trustedProxy, | ||
| hint: `Expected "cloudflare", "none", or a hop count 1-${MAX_TRUSTED_HOPS}`, | ||
| }) | ||
| } | ||
|
|
||
| // Default: use the socket remote address (cannot be spoofed) | ||
| return getRequestIP(event) ?? '0.0.0.0' | ||
| if (trustedPeer) return { kind: 'trusted-peer', ip: trustedPeer } | ||
|
|
||
| // Hosted reqcore serves every request through Cloudflare → Railway, so the | ||
| // socket peer is the platform edge and is identical for all traffic. Keying on | ||
| // it would put the entire internet in one bucket — the 6th applicant across | ||
| // all orgs would get a 429. Default to the Cloudflare header in production; | ||
| // deployments exposed directly to the internet must set TRUSTED_PROXY=none, | ||
| // otherwise a client can pick its own bucket by sending CF-Connecting-IP. | ||
| if (process.env.NODE_ENV === 'production') return { kind: 'cloudflare' } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unvalidated TRUSTED_PROXY values fail open to cloudflare in production. The root cause is that no layer rejects a bad value: the env schema accepts any string, and the runtime parser warns and then falls through to the production default, which trusts CF-Connecting-IP. On a directly-exposed host where the operator intended none, a typo lets clients choose their own rate-limit bucket.
server/utils/rateLimit.ts#L191-L205: after loggingrate_limit.trusted_proxy_invalid, don't fall through to the productioncloudflaredefault — either document the fail-open decision explicitly in the comment block or treat an invalid explicit value as fatal.server/utils/env.ts#L89-L98: constrainTRUSTED_PROXYtocloudflare | none | 1-10so a bad value fails at startup instead of degrading silently at request time.
📍 Affects 2 files
server/utils/rateLimit.ts#L191-L205(this comment)server/utils/env.ts#L89-L98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/utils/rateLimit.ts` around lines 191 - 205, Prevent invalid explicit
TRUSTED_PROXY values from falling through to the production Cloudflare default.
In server/utils/rateLimit.ts lines 191-205, make the invalid-value path fatal
after logging rate_limit.trusted_proxy_invalid, or explicitly document the
intentional fail-open behavior; in server/utils/env.ts lines 89-98, constrain
TRUSTED_PROXY validation to cloudflare, none, or numeric hop counts 1-10 so
invalid configuration fails during startup.
| function forwardedIpFromRight(event: H3Event, hops: number): string | undefined { | ||
| const entries = forwardedEntries(event) | ||
| if (entries.length === 0) return undefined | ||
|
|
||
| const index = Math.max(0, entries.length - hops) | ||
| return normalizeIp(entries[index]) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Clamping the hop index to 0 trusts a client-supplied X-Forwarded-For entry.
When entries.length < hops, Math.max(0, …) silently selects the leftmost entry. With TRUSTED_PROXY=2 but only one proxy actually appending, a client sending X-Forwarded-For: <chosen-ip> produces two entries and index 0 resolves to the value the client chose — self-selected rate-limit bucket. Returning undefined instead falls through to the socket key and emits the existing rate_limit.socket_ip_fallback warning, which also surfaces the misconfiguration.
🛡️ Proposed fix
function forwardedIpFromRight(event: H3Event, hops: number): string | undefined {
const entries = forwardedEntries(event)
- if (entries.length === 0) return undefined
-
- const index = Math.max(0, entries.length - hops)
- return normalizeIp(entries[index])
+ // Fewer entries than trusted hops means the chain is shorter than configured;
+ // the leftmost entry would then be client-written, so refuse it.
+ if (entries.length < hops) return undefined
+
+ return normalizeIp(entries[entries.length - hops])
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function forwardedIpFromRight(event: H3Event, hops: number): string | undefined { | |
| const entries = forwardedEntries(event) | |
| if (entries.length === 0) return undefined | |
| const index = Math.max(0, entries.length - hops) | |
| return normalizeIp(entries[index]) | |
| } | |
| function forwardedIpFromRight(event: H3Event, hops: number): string | undefined { | |
| const entries = forwardedEntries(event) | |
| // Fewer entries than trusted hops means the chain is shorter than configured; | |
| // the leftmost entry would then be client-written, so refuse it. | |
| if (entries.length < hops) return undefined | |
| return normalizeIp(entries[entries.length - hops]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/utils/rateLimit.ts` around lines 295 - 301, Update
forwardedIpFromRight to return undefined when hops exceeds entries.length
instead of clamping the index to zero. Only normalize and return the selected
forwarded entry when the configured hop count is available, preserving the
empty-entry behavior so callers fall back to the socket key and warning.
| if (newTitle && currentStatus === 'draft') { | ||
| return generateJobSlug(newTitle, id) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid regenerating a draft slug for an unchanged title.
newTitle is not compared with currentTitle. A draft that already has a custom slug will lose it when an edit submits the same title without customSlug. Only regenerate when the title actually changed; add a same-title regression test.
Proposed fix
- if (newTitle && currentStatus === 'draft') {
+ if (newTitle && newTitle !== currentTitle && currentStatus === 'draft') {
return generateJobSlug(newTitle, id)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (newTitle && currentStatus === 'draft') { | |
| return generateJobSlug(newTitle, id) | |
| if (newTitle && newTitle !== currentTitle && currentStatus === 'draft') { | |
| return generateJobSlug(newTitle, id) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/utils/slugify.ts` around lines 53 - 54, Update the draft branch in
generateJobSlug to regenerate only when newTitle differs from currentTitle,
preserving an existing custom slug when the submitted title is unchanged. Add a
regression test covering an unchanged draft title without customSlug and verify
the custom slug remains intact.
|
🚅 Deployed to the reqcore-pr-245 environment in applirank
|
1 participant
TRUSTED_PROXYenvironment variable to configure client IP resolution for rate limiting and add documentation for common setups.Summary
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
New Features
Bug Fixes
Documentation