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.
📝 WalkthroughWalkthroughThe PR adds configurable trusted-proxy rate limiting, structured application validation, conditional job slug updates, markdown normalization and rendering styles, client-side input limits, documentation, syntax repair, and unit-test coverage. ChangesProxy-aware rate limiting
Public application validation
Conditional job slug updates
Recruiter markdown rendering
Dashboard syntax repair
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Applicant
participant ApplicationForm
participant ApplicationEndpoint
participant parsePublicApplication
Applicant->>ApplicationForm: submit application and tracking values
ApplicationForm->>ApplicationEndpoint: send multipart or JSON payload
ApplicationEndpoint->>parsePublicApplication: validate normalized input
parsePublicApplication-->>ApplicationEndpoint: parsed data or structured 400 error
ApplicationEndpoint-->>ApplicationForm: success or field-specific error
ApplicationForm->>Applicant: display inline validation message
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
|
…imit-slug-validation # Conflicts: # server/api/jobs/[id].patch.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/rate-limit.test.ts (1)
240-243: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test passes for the wrong reason — the risky path is uncovered.
NODE_ENVhere is the test default, so an invalidTRUSTED_PROXYyieldssocketvia the non-production default. UnderNODE_ENV=productionthe same input falls through tocloudflareand does trust headers. Add that case (plus ahops> XFF-entry-count case) so the fail-open behavior flagged inserver/utils/rateLimit.tsis pinned by a test.💚 Suggested additional cases
it('ignores an invalid TRUSTED_PROXY value instead of trusting headers blindly', () => { process.env.TRUSTED_PROXY = 'yes-please' expect(getClientIp(makeEvent('100.64.0.3', { 'cf-connecting-ip': '203.0.113.7' }))).toBe('100.64.0.3') }) + + it('does not trust CF-Connecting-IP in production when TRUSTED_PROXY is invalid', () => { + process.env.NODE_ENV = 'production' + process.env.TRUSTED_PROXY = 'yes-please' + expect(getClientIp(makeEvent('100.64.0.3', { 'cf-connecting-ip': '203.0.113.7' }))).toBe('100.64.0.3') + }) + + it('ignores X-Forwarded-For when the chain is shorter than the trusted hop count', () => { + process.env.TRUSTED_PROXY = '2' + expect(getClientIp(makeEvent('10.0.0.1', { 'x-forwarded-for': '1.1.1.1' }))).toBe('10.0.0.1') + })🤖 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 240 - 243, Extend the rate-limit tests around the invalid TRUSTED_PROXY case to run with NODE_ENV set to production and assert that invalid configuration does not select the cloudflare/header-trusting path, then restore the environment afterward. Add coverage for a configured hops value exceeding the available X-Forwarded-For entries and assert the client IP remains safe; keep the existing valid configuration tests unchanged.
🤖 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 18-29: Update firstQueryValue to trim candidate string values
before validating them, treating whitespace-only entries as unusable and
continuing through arrays to find the first non-empty trimmed value. Return the
trimmed value so the source tracking parameters match the schema’s trimmed-value
behavior.
In `@server/utils/schemas/publicApplication.ts`:
- Around line 43-44: Update the firstName and lastName validators in the public
application schema to trim surrounding whitespace before applying the required
and maximum-length checks, ensuring whitespace-only JSON values fail validation
while preserving existing error messages and limits.
---
Nitpick comments:
In `@tests/unit/rate-limit.test.ts`:
- Around line 240-243: Extend the rate-limit tests around the invalid
TRUSTED_PROXY case to run with NODE_ENV set to production and assert that
invalid configuration does not select the cloudflare/header-trusting path, then
restore the environment afterward. Add coverage for a configured hops value
exceeding the available X-Forwarded-For entries and assert the client IP remains
safe; keep the existing valid configuration tests unchanged.
🪄 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: b9983d38-a88e-4461-ab4c-ef8cf6386288
📒 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
| function firstQueryValue(value: unknown): string | undefined { | ||
| const raw = Array.isArray(value) ? value.find((v) => typeof v === 'string' && v.length > 0) : value | ||
| return typeof raw === 'string' && raw.length > 0 ? raw : undefined | ||
| } | ||
|
|
||
| // Capture source tracking params from the URL | ||
| const sourceRef = (route.query.ref as string) || undefined | ||
| const utmSource = (route.query.utm_source as string) || undefined | ||
| const utmMedium = (route.query.utm_medium as string) || undefined | ||
| const utmCampaign = (route.query.utm_campaign as string) || undefined | ||
| const utmTerm = (route.query.utm_term as string) || undefined | ||
| const utmContent = (route.query.utm_content as string) || undefined | ||
| const sourceRef = firstQueryValue(route.query.ref) | ||
| const utmSource = firstQueryValue(route.query.utm_source) | ||
| const utmMedium = firstQueryValue(route.query.utm_medium) | ||
| const utmCampaign = firstQueryValue(route.query.utm_campaign) | ||
| const utmTerm = firstQueryValue(route.query.utm_term) | ||
| const utmContent = firstQueryValue(route.query.utm_content) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat whitespace-only query values as unusable.
Line 19 selects ' ' as the first value, then the server drops it and never sees a later usable value such as ?ref=%20%20%20&ref=linkedin. Match the schema’s trimmed-value behavior here.
Proposed fix
function firstQueryValue(value: unknown): string | undefined {
- const raw = Array.isArray(value) ? value.find((v) => typeof v === 'string' && v.length > 0) : value
- return typeof raw === 'string' && raw.length > 0 ? raw : undefined
+ const raw = Array.isArray(value)
+ ? value.find((v) => typeof v === 'string' && v.trim().length > 0)
+ : value
+ return typeof raw === 'string' && raw.trim().length > 0 ? raw.trim() : undefined
}📝 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 firstQueryValue(value: unknown): string | undefined { | |
| const raw = Array.isArray(value) ? value.find((v) => typeof v === 'string' && v.length > 0) : value | |
| return typeof raw === 'string' && raw.length > 0 ? raw : undefined | |
| } | |
| // Capture source tracking params from the URL | |
| const sourceRef = (route.query.ref as string) || undefined | |
| const utmSource = (route.query.utm_source as string) || undefined | |
| const utmMedium = (route.query.utm_medium as string) || undefined | |
| const utmCampaign = (route.query.utm_campaign as string) || undefined | |
| const utmTerm = (route.query.utm_term as string) || undefined | |
| const utmContent = (route.query.utm_content as string) || undefined | |
| const sourceRef = firstQueryValue(route.query.ref) | |
| const utmSource = firstQueryValue(route.query.utm_source) | |
| const utmMedium = firstQueryValue(route.query.utm_medium) | |
| const utmCampaign = firstQueryValue(route.query.utm_campaign) | |
| const utmTerm = firstQueryValue(route.query.utm_term) | |
| const utmContent = firstQueryValue(route.query.utm_content) | |
| function firstQueryValue(value: unknown): string | undefined { | |
| const raw = Array.isArray(value) | |
| ? value.find((v) => typeof v === 'string' && v.trim().length > 0) | |
| : value | |
| return typeof raw === 'string' && raw.trim().length > 0 ? raw.trim() : undefined | |
| } |
🤖 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 18 - 29, Update firstQueryValue
to trim candidate string values before validating them, treating whitespace-only
entries as unusable and continuing through arrays to find the first non-empty
trimmed value. Return the trimmed value so the source tracking parameters match
the schema’s trimmed-value behavior.
| firstName: z.string().min(1, 'First name is required').max(100, 'Must be 100 characters or fewer'), | ||
| lastName: z.string().min(1, 'Last name is required').max(100, 'Must be 100 characters or fewer'), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Trim required names before validating them.
Line 43 and Line 44 accept whitespace-only JSON values because .min(1) counts spaces. Multipart input is trimmed upstream, but JSON is passed directly to this schema, so the two submission paths can persist blank applicant names.
Proposed fix
- firstName: z.string().min(1, 'First name is required').max(100, 'Must be 100 characters or fewer'),
- lastName: z.string().min(1, 'Last name is required').max(100, 'Must be 100 characters or fewer'),
+ firstName: z.string().trim().min(1, 'First name is required').max(100, 'Must be 100 characters or fewer'),
+ lastName: z.string().trim().min(1, 'Last name is required').max(100, 'Must be 100 characters or fewer'),📝 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.
| firstName: z.string().min(1, 'First name is required').max(100, 'Must be 100 characters or fewer'), | |
| lastName: z.string().min(1, 'Last name is required').max(100, 'Must be 100 characters or fewer'), | |
| firstName: z.string().trim().min(1, 'First name is required').max(100, 'Must be 100 characters or fewer'), | |
| lastName: z.string().trim().min(1, 'Last name is required').max(100, 'Must be 100 characters or fewer'), |
🤖 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/schemas/publicApplication.ts` around lines 43 - 44, Update the
firstName and lastName validators in the public application schema to trim
surrounding whitespace before applying the required and maximum-length checks,
ensuring whitespace-only JSON values fail validation while preserving existing
error messages and limits.
- Extract slug logic into shared `job-slug.ts` to unify base generation, suffix stripping, and preview formatting across the stack. - Improve slug generation to support international titles. - Add collision handling for job creation and updates by retrying with a fresh suffix if a conflict occurs. - Update `createRateLimiter` to support a `successes` count mode, ensuring rejected form submissions do not consume an applicant's rate limit.
Implement `job-prose` styles for rendering markdown content, replacing `@tailwindcss/typography` to ensure consistent rendering of recruiter-pasted content. Added `normalizeDescriptionMarkdown` to sanitize pasted text (fixing indent-to-code-block issues) and a `dense` mode for chat contexts.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/rateLimit.ts (1)
175-202: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftReserve capacity for in-flight successful requests.
In
successesmode, concurrent requests all pass Line 175 before any handler reachesrecord()at Line 198. Since the public application endpoint uses this mode, a parallel burst can exceedmaxRequestsby an arbitrary amount. Track pending reservations during enforcement and release them when submission fails, or make successful accounting atomic.🤖 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 175 - 202, Update the rate-limit enforcement around the returned `RateLimiter` and its `record` method so `successes` mode reserves capacity before allowing a request to proceed, preventing concurrent requests from all passing the limit check. Track pending reservations per resolved client key, include them in the limit calculation, and release a reservation when submission fails while converting it to successful accounting in `record`; preserve the existing `attempts` behavior.
🤖 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/dashboard/jobs/`[id]/settings.vue:
- Line 1: Remove the stray “conca” prefix before the script setup block in the
component, leaving the opening tag exactly as `<script setup lang="ts">` so the
Vue SFC parses correctly.
In `@server/api/public/jobs/`[slug]/apply.post.ts:
- Around line 32-35: Make the success-only admission flow atomic: update
createRateLimiter and rateLimit so concurrent requests reserve capacity before
processing, then commit the reservation only on successful submission and
release it on validation or other failure paths; ensure the combined recorded
and in-flight count never exceeds maxRequests. Update both deferred record()
call sites near the success paths to use this reservation lifecycle without
counting validation failures.
In `@server/utils/slugify.ts`:
- Around line 47-68: The draft branch in resolveJobSlugUpdate must only
regenerate a slug when newTitle differs from currentTitle; add that comparison
while preserving the publishedAt check. In tests/unit/job-slug.test.ts lines
75-145, add a regression case asserting undefined for an unpublished job with
identical currentTitle and newTitle and an existing custom slug.
---
Outside diff comments:
In `@server/utils/rateLimit.ts`:
- Around line 175-202: Update the rate-limit enforcement around the returned
`RateLimiter` and its `record` method so `successes` mode reserves capacity
before allowing a request to proceed, preventing concurrent requests from all
passing the limit check. Track pending reservations per resolved client key,
include them in the limit calculation, and release a reservation when submission
fails while converting it to successful accounting in `record`; preserve the
existing `attempts` behavior.
🪄 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: 451dbf88-e597-4810-9b76-4537db7e1850
📒 Files selected for processing (16)
app/assets/css/main.cssapp/components/MarkdownDescription.vueapp/composables/useJob.tsapp/pages/dashboard/chatbot/[[id]].vueapp/pages/dashboard/jobs/[id]/settings.vueapp/pages/dashboard/jobs/new.vueserver/api/jobs/[id].patch.tsserver/api/jobs/index.post.tsserver/api/public/jobs/[slug]/apply.post.tsserver/utils/rateLimit.tsserver/utils/slugify.tsshared/job-description.tsshared/job-slug.tstests/unit/job-description-normalize.test.tstests/unit/job-slug.test.tstests/unit/rate-limit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/rate-limit.test.ts
| @@ -1,4 +1,4 @@ | |||
| <script setup lang="ts"> | |||
| conca<script setup lang="ts"> | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="app/pages/dashboard/jobs/[id]/settings.vue"
if [ -f "$file" ]; then
echo "== first 10 lines =="
cat -n "$file" | sed -n '1,10p'
else
echo "File not found: $file"
echo "Matching settings.vue files:"
fd -a 'settings\.vue$' . | sed "s#$(pwd)/##" | head -50
fi
echo "== search for malformed conca/script tag-like syntax in tracked files =="
git ls-files | xargs rg -n '<script\s|conca|<script' --glob '*.vue' --glob '*.ts' --glob '*.js' || trueRepository: reqcore-inc/reqcore
Length of output: 12688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path("app/pages/dashboard/jobs/[id]/settings.vue").read_text()
first_line = text.splitlines()[0]
script_tag = "<script setup lang=\"ts\">"
print(f"first_line_matches='<script setup lang=\"ts\">': {first_line == script_tag}")
print(f"starts_with_conca_prefix: {first_line.startswith('conca')}")
print(f"contains_expected_script_tag_after_conca: {'<script setup lang=\"ts\">' in first_line}")
PYRepository: reqcore-inc/reqcore
Length of output: 449
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
s = Path("app/pages/dashboard/jobs/[id]/settings.vue").read_text()
first_line = s.splitlines()[0]
expected = '<script setup lang="ts">'
print(f"first_line_matches=<script setup lang='ts'>: {first_line == expected}")
print(f"starts_with_conca_prefix: {first_line.startswith('conca')}")
print(f"contains_expected_script_tag_after_conca: {expected in first_line}")
PYRepository: reqcore-inc/reqcore
Length of output: 286
Remove the stray conca prefix.
app/pages/dashboard/jobs/[id]/settings.vue:1 currently reads conca<script setup lang="ts">, which is invalid Vue SFC syntax and will prevent the component from compiling. It should be exactly <script setup lang="ts">.
🤖 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/dashboard/jobs/`[id]/settings.vue at line 1, Remove the stray
“conca” prefix before the script setup block in the component, leaving the
opening tag exactly as `<script setup lang="ts">` so the Vue SFC parses
correctly.
| const applyRateLimit = createRateLimiter({ | ||
| windowMs: 15 * 60 * 1000, | ||
| maxRequests: 5, | ||
| windowMs: 60 * 60 * 1000, | ||
| maxRequests: 30, | ||
| countMode: 'successes', |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Make success-only rate-limit admission atomic.
With countMode: 'successes', rateLimit(event) only checks the current bucket; recording is deferred until Line 192 or Line 736. If 29 of 30 slots are used, concurrent submissions can all pass before any record() call, then push the bucket beyond 30 because record() does not re-check the limit.
Add an atomic in-flight reservation/commit-release API to the limiter, or use attempt counting if a strict cap is required. Moving record() earlier would incorrectly count validation failures.
Also applies to: 187-192, 733-737
🤖 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/api/public/jobs/`[slug]/apply.post.ts around lines 32 - 35, Make the
success-only admission flow atomic: update createRateLimiter and rateLimit so
concurrent requests reserve capacity before processing, then commit the
reservation only on successful submission and release it on validation or other
failure paths; ensure the combined recorded and in-flight count never exceeds
maxRequests. Update both deferred record() call sites near the success paths to
use this reservation lifecycle without counting validation failures.
| export function resolveJobSlugUpdate(args: { | ||
| id: string | ||
| publishedAt: Date | null | ||
| currentTitle: string | ||
| newTitle?: string | ||
| customSlug?: string | ||
| }): string | undefined { | ||
| const { id, publishedAt, currentTitle, newTitle, customSlug } = args | ||
|
|
||
| // An explicit slug is the recruiter asking for the rename on purpose. Callers | ||
| // must only send one the recruiter actually changed — sending back whatever | ||
| // was loaded into the field reads as a deliberate rename on every save. | ||
| if (customSlug?.trim()) { | ||
| return generateJobSlug(newTitle ?? currentTitle, id, customSlug) | ||
| } | ||
|
|
||
| if (newTitle && publishedAt === null) { | ||
| return generateJobSlug(newTitle, id) | ||
| } | ||
|
|
||
| return undefined | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing newTitle !== currentTitle guard, and no regression test for it. resolveJobSlugUpdate's draft branch (newTitle && publishedAt === null) regenerates the slug from the title with no comparison to currentTitle, so resaving a never-published job with its title unchanged silently overwrites a recruiter-set custom slug. The test suite doesn't exercise this case either.
server/utils/slugify.ts#L47-L68: addnewTitle !== currentTitleto the draft-branch condition so an unchanged title never regenerates the slug.tests/unit/job-slug.test.ts#L75-L145: add a case assertingresolveJobSlugUpdatereturnsundefinedwhenpublishedAt: null,newTitle === currentTitle, and acustomSlugwas previously set (i.e., the stored slug isn't title-derived).
📍 Affects 2 files
server/utils/slugify.ts#L47-L68(this comment)tests/unit/job-slug.test.ts#L75-L145
🤖 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 47 - 68, The draft branch in
resolveJobSlugUpdate must only regenerate a slug when newTitle differs from
currentTitle; add that comparison while preserving the publishedAt check. In
tests/unit/job-slug.test.ts lines 75-145, add a regression case asserting
undefined for an unpublished job with identical currentTitle and newTitle and an
existing custom slug.
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