Skip to content
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.

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

fix(js): extend and add open redirect rules #309

Merged
merged 2 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .github/workflows/canary_integration_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ jobs:
group:
[
"javascript/express",
"javascript/hapi",
"javascript/lang",
"javascript/react",
"javascript/third_parties",
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/integration_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ jobs:
group:
[
"javascript/express",
"javascript/hapi",
"javascript/lang",
"javascript/react",
"javascript/third_parties",
Expand Down
57 changes: 57 additions & 0 deletions rules/javascript/hapi/open_redirect.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
imports:
- javascript_shared_common_user_input
patterns:
- pattern: |
$<RES>.redirect($<USER_INPUT>$<...>)
filters:
- variable: RES
detection: javascript_hapi_open_redirect_response_toolkit
- variable: USER_INPUT
detection: javascript_shared_common_user_input
scope: result
auxiliary:
- id: javascript_hapi_open_redirect_response_toolkit
patterns:
- responseToolkit
# typescript
- |
const $<_>: ResponseToolkit = $<!>$<_>
- |
($<...>$<!>$<_>: ResponseToolkit$<...>) => {}
- |
($<...>$<!>$<_>: ResponseToolkit$<...>) {}
- |
function ($<...>$<!>$<_>: ResponseToolkit$<...>) {}
- |
function $<_>($<...>$<!>$<_>: ResponseToolkit$<...>) {}
- |
class $<_> $<...>{ $<...>$<_>($<...>$<!>$<_>: ResponseToolkit$<...>) {} }
languages:
- javascript
severity: medium
metadata:
description: Unsanitized user input in redirect
remediation_message: |
## Description
A redirect using unsanitized user input is bad practice and puts your application at greater risk of phishing attacks.

## Remediations
❌ Do not use unsanitized user input when constructing URLs

✅ Instead, ensure the input is validated by using a safe list or a mapping when constructing URLs

```javascript
var map = {
"1": "/planes",
"2": "/trains",
"3": "/automobiles",
}

res.redirect(map[req.body.transport])
```
## Resources
- [OWASP open redirect](https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html)
cwe_id:
- 601
id: javascript_hapi_open_redirect
documentation_url: https://docs.bearer.com/reference/rules/javascript_hapi_open_redirect
9 changes: 9 additions & 0 deletions rules/javascript/shared/express/user_input.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ patterns:
- headers
- params
- query
- url
- pattern: const { $<!>$<PROPERTY> } = $<REQUEST>
filters:
- variable: REQUEST
Expand All @@ -26,6 +27,7 @@ patterns:
- headers
- params
- query
- url
# typescript
- pattern: |
const { $<!>$<PROPERTY> }: Request = $<_>
Expand All @@ -37,6 +39,7 @@ patterns:
- headers
- params
- query
- url
- pattern: |
($<...>{ $<!>$<PROPERTY> }: Request$<...>) => {}
filters:
Expand All @@ -47,6 +50,7 @@ patterns:
- headers
- params
- query
- url
- pattern: |
function ($<...>{ $<!>$<PROPERTY> }: Request$<...>) {}
filters:
Expand All @@ -57,6 +61,7 @@ patterns:
- headers
- params
- query
- url
- pattern: |
function $<_>($<...>{ $<!>$<PROPERTY> }: Request$<...>) {}
filters:
Expand All @@ -67,6 +72,7 @@ patterns:
- headers
- params
- query
- url
- pattern: |
class $<_> $<...>{ $<_>($<...>{ $<!>$<PROPERTY> }: Request$<...>) {} }
filters:
Expand All @@ -77,6 +83,7 @@ patterns:
- headers
- params
- query
- url
auxiliary:
- id: javascript_shared_express_user_input_request
patterns:
Expand All @@ -86,6 +93,8 @@ auxiliary:
const $<_>: Request = $<!>$<_>
- |
($<...>$<!>$<_>: Request$<...>) => {}
- |
($<...>$<!>$<_>: Request$<...>) {}
- |
function ($<...>$<!>$<_>: Request$<...>) {}
- |
Expand Down
15 changes: 12 additions & 3 deletions tests/javascript/express/open_redirect/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const { ruleId, ruleFile, testBase } = getEnvironment(__dirname)
describe(ruleId, () => {
const invoke = createNewInvoker(ruleId, ruleFile, testBase)


test("ok_no_open_redirect", () => {
const testCase = "ok_no_open_redirect.js"

Expand All @@ -16,7 +16,7 @@ describe(ruleId, () => {
expect(results.Missing).toEqual([])
expect(results.Extra).toEqual([])
})


test("open_redirect", () => {
const testCase = "open_redirect.js"
Expand All @@ -26,5 +26,14 @@ describe(ruleId, () => {
expect(results.Missing).toEqual([])
expect(results.Extra).toEqual([])
})


test("open_redirect_ts", () => {
const testCase = "app.ts"

const results = invoke(testCase)

expect(results.Missing).toEqual([])
expect(results.Extra).toEqual([])
})

})
12 changes: 12 additions & 0 deletions tests/javascript/express/open_redirect/testdata/app.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { format as formatUrl } from 'url';

export class Foo {
public async bad(req: Request, res: Response) {
// bearer:expected javascript_express_open_redirect
return res.redirect(formatUrl({
pathname: req.url.pathname
})
)
.takeover();
}
}
27 changes: 27 additions & 0 deletions tests/javascript/hapi/open_redirect/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
const {
createNewInvoker,
getEnvironment,
} = require("../../../helper.js")
const { ruleId, ruleFile, testBase } = getEnvironment(__dirname)

describe(ruleId, () => {
const invoke = createNewInvoker(ruleId, ruleFile, testBase)

test("open_redirect_js", () => {
const testCase = "app.js"

const results = invoke(testCase)

expect(results.Missing).toEqual([])
expect(results.Extra).toEqual([])
})

test("open_redirect_ts", () => {
const testCase = "app.ts"

const results = invoke(testCase)

expect(results.Missing).toEqual([])
expect(results.Extra).toEqual([])
})
})
14 changes: 14 additions & 0 deletions tests/javascript/hapi/open_redirect/testdata/app.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { Request, ResponseToolkit } from "@hapi/hapi";
import { format as formatUrl } from 'url';

export class Foo {
async bad(req, responseToolkit) {
// bearer:expected javascript_hapi_open_redirect
return responseToolkit
.redirect(formatUrl({
pathname: req.url.pathname
})
)
.takeover();
}
}
14 changes: 14 additions & 0 deletions tests/javascript/hapi/open_redirect/testdata/app.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { Request, ResponseToolkit } from "@hapi/hapi";
import { format as formatUrl } from 'url';

export class Foo {
public async bad(req: Request, res: ResponseToolkit) {
// bearer:expected javascript_hapi_open_redirect
return res
.redirect(formatUrl({
pathname: req.url.pathname
})
)
.takeover();
}
}
Loading