Skip to content

Guard against parsing non-JSON responses as JSON #8491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iHiD wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Guard against parsing non-JSON responses as JSON #8491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/javascript/bootcamp/return.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@
const response = await fetch(
`/courses/stripe/session-status?session_id=${sessionId}&enrollment_uuid=${enrollmentUuid}`
)

if (!response.ok) {
window.location.replace(failurePath)

Check failure

Code scanning / CodeQL

Client-side cross-site scripting High

Cross-site scripting vulnerability due to
user-provided value
Loading
.

Copilot Autofix

AI about 2 months ago

Copilot could not generate an autofix suggestion

Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.

Check warning

Code scanning / CodeQL

Client-side URL redirect Medium

Untrusted URL redirection depends on a
user-provided value
Loading
.

Copilot Autofix

AI about 2 months ago

In general, to fix client-side URL redirect issues, avoid redirecting directly to arbitrary strings derived from user input. Instead, restrict redirects to a small, explicit allowlist of known-safe paths or route identifiers, and map any unrecognized value to a safe default.

For this snippet, the best fix that preserves behavior is to keep reading failure_path from the query string but validate it against a predefined set of allowed internal paths under /courses/. We can do that fully on the client by defining a small allowlist array (e.g., SAFE_FAILURE_PATHS) and only using a failurePath that exactly matches one of those entries. If the user-supplied failure_path is missing or not in the allowlist, we fall back to the existing /bootcamp default. This removes the direct dependency of the redirect target on untrusted data while still supporting the intended, legitimate redirection cases.

Concretely, in app/javascript/bootcamp/return.js:

  • Above or inside initialize(), define a SAFE_FAILURE_PATHS constant with the specific paths you want to allow, such as ['/courses/enrolled', '/courses'] (you should adjust this list to the project’s real expected values).
  • Replace the current failurePath logic:
    • Keep retrieving failurePath from urlParams.
    • Remove the startsWith('/courses/') check.
    • Add a helper function or inline logic that checks whether failurePath is one of the entries in SAFE_FAILURE_PATHS. If not, set failurePath = '/bootcamp'.
  • Leave the subsequent uses of failurePath (lines 16 and 24) unchanged; they will now only ever use either an allowlisted path or /bootcamp.

This change adds only a small constant and, optionally, a helper function; no extra imports or external libraries are required.

Suggested changeset 1
app/javascript/bootcamp/return.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/javascript/bootcamp/return.js b/app/javascript/bootcamp/return.js
--- a/app/javascript/bootcamp/return.js
+++ b/app/javascript/bootcamp/return.js
@@ -2,13 +2,15 @@
   initialize()
 }
 
+const SAFE_FAILURE_PATHS = ['/courses/enrolled']
+
 async function initialize() {
   const queryString = window.location.search
   const urlParams = new URLSearchParams(queryString)
   const sessionId = urlParams.get('session_id')
   const enrollmentUuid = urlParams.get('enrollment_uuid')
   let failurePath = urlParams.get('failure_path')
-  if (!failurePath || !failurePath.startsWith('/courses/')) {
+  if (!failurePath || !SAFE_FAILURE_PATHS.includes(failurePath)) {
     failurePath = '/bootcamp'
   }
 
EOF
@@ -2,13 +2,15 @@
initialize()
}

const SAFE_FAILURE_PATHS = ['/courses/enrolled']

async function initialize() {
const queryString = window.location.search
const urlParams = new URLSearchParams(queryString)
const sessionId = urlParams.get('session_id')
const enrollmentUuid = urlParams.get('enrollment_uuid')
let failurePath = urlParams.get('failure_path')
if (!failurePath || !failurePath.startsWith('/courses/')) {
if (!failurePath || !SAFE_FAILURE_PATHS.includes(failurePath)) {
failurePath = '/bootcamp'
}

Copilot is powered by AI and may make mistakes. Always verify output.
return
}

const session = await response.json()

if (session.status == 'open') {
Expand Down
32 changes: 19 additions & 13 deletions app/javascript/components/ErrorBoundary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,19 +94,25 @@ export const useErrorHandler = (

handler(new HandledError(defaultError.message))
} else if (error instanceof Response) {
error
.clone()
.json()
.then((res: { error: APIError }) => {
handler(new HandledError(res.error.message))
})
.catch((e) => {
if (process.env.NODE_ENV == 'production') {
Sentry.captureException(e)
}

handler(new HandledError(defaultError.message))
})
const contentType = error.headers.get('Content-Type')
const isJson =
contentType &&
(contentType.includes('application/json') ||
contentType.includes('+json'))

if (isJson) {
error
.clone()
.json()
.then((res: { error: APIError }) => {
handler(new HandledError(res.error.message))
})
.catch(() => {
handler(new HandledError(defaultError.message))
})
} else {
handler(new HandledError(defaultError.message))
}
}
}, [defaultError, error, handler])
}
Expand Down
1 change: 1 addition & 0 deletions app/javascript/components/profile/avatar-selector/cropping-modal/CropFinishedStep.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export const CropFinishedStep = ({
/* TODO: (optional) Use our standard sendRequest library */
return fetch(links.update, { body: formData, method: 'PATCH' })
.then((response) => {
if (!response.ok) throw response
return response.json().then((json) => camelizeKeys(json))
})
.then((json) => {
Expand Down
5 changes: 4 additions & 1 deletion app/javascript/hooks/use-theme-observer/patch-theme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ function patchTheme(theme: string, updateEndpoint?: string) {
user_preferences: { theme },
}),
})
.then((res) => res.json())
.then((res) => {
if (!res.ok) throw new Error('Failed to update theme')
return res.json()
})
.catch((e) =>
// eslint-disable-next-line no-console
console.error('Failed to update to accessibility-dark theme: ', e)
Expand Down
Loading