Skip to content

Commit 35eb321

Browse files
tusharpandey13Simen A. W. Olsen
andcommitted
fix: prevent OAuth parameter injection via returnTo (#2381)
- URL encode returnTo parameter to prevent injection of OAuth parameters - Add comprehensive unit tests for returnTo encoding scenarios - Tests cover basic encoding, OAuth param injection attempts, and edge cases Co-authored-by: Simen A. W. Olsen <[email protected]>
1 parent 0055cc4 commit 35eb321

File tree

3 files changed

+90
-4
lines changed

3 files changed

+90
-4
lines changed

‎.gitignore‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,4 +141,5 @@ dist
141141
*.tmp
142142
*PLAN*.md
143143
.yalc/
144-
yalc.lock
144+
yalc.lock
145+
.npmrc

‎src/server/helpers/with-page-auth-required.test.ts‎

Lines changed: 87 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ describe("with-page-auth-required ssr", () =>{
8181
);
8282
awaitexpect(handler({})).rejects.toThrowError("NEXT_REDIRECT");
8383
expect(redirect).toHaveBeenCalledTimes(1);
84-
expect(redirect).toHaveBeenCalledWith("/auth/login?returnTo=/foo");
84+
expect(redirect).toHaveBeenCalledWith("/auth/login?returnTo=%2Ffoo");
8585
});
8686

8787
it("should protect a page and redirect to returnTo fn option",async()=>{
@@ -114,7 +114,7 @@ describe("with-page-auth-required ssr", () =>{
114114
).rejects.toThrowError("NEXT_REDIRECT");
115115
expect(redirect).toHaveBeenCalledTimes(1);
116116
expect(redirect).toHaveBeenCalledWith(
117-
"/auth/login?returnTo=/foo/bar?foo=bar"
117+
"/auth/login?returnTo=%2Ffoo%2Fbar%3Ffoo%3Dbar"
118118
);
119119
});
120120

@@ -165,6 +165,91 @@ describe("with-page-auth-required ssr", () =>{
165165
expect(redirect).toHaveBeenCalledTimes(1);
166166
expect(redirect).toHaveBeenCalledWith("/api/auth/custom-login");
167167
});
168+
169+
it("should URL encode returnTo parameter to prevent OAuth param injection",async()=>{
170+
constwithPageAuthRequired=appRouteHandlerFactory(
171+
newAuth0Client({
172+
domain: constants.domain,
173+
clientId: constants.clientId,
174+
clientSecret: constants.clientSecret,
175+
appBaseUrl: constants.appBaseUrl,
176+
secret: constants.secret
177+
}),
178+
{
179+
loginUrl: "/auth/login"
180+
}
181+
);
182+
consthandler=withPageAuthRequired(
183+
()=>Promise.resolve(React.createElement("div",{},"foo")),
184+
{
185+
returnTo:
186+
"/callback?scope=openid profile&audience=https://api.example.com"
187+
}
188+
);
189+
awaitexpect(handler({})).rejects.toThrowError("NEXT_REDIRECT");
190+
expect(redirect).toHaveBeenCalledTimes(1);
191+
expect(redirect).toHaveBeenCalledWith(
192+
"/auth/login?returnTo=%2Fcallback%3Fscope%3Dopenid%20profile%26audience%3Dhttps%3A%2F%2Fapi.example.com"
193+
);
194+
});
195+
196+
it("should URL encode returnTo with special characters",async()=>{
197+
constwithPageAuthRequired=appRouteHandlerFactory(
198+
newAuth0Client({
199+
domain: constants.domain,
200+
clientId: constants.clientId,
201+
clientSecret: constants.clientSecret,
202+
appBaseUrl: constants.appBaseUrl,
203+
secret: constants.secret
204+
}),
205+
{
206+
loginUrl: "/auth/login"
207+
}
208+
);
209+
consthandler=withPageAuthRequired(
210+
()=>Promise.resolve(React.createElement("div",{},"foo")),
211+
{
212+
returnTo: "/path/with spaces & special=chars"
213+
}
214+
);
215+
awaitexpect(handler({})).rejects.toThrowError("NEXT_REDIRECT");
216+
expect(redirect).toHaveBeenCalledTimes(1);
217+
expect(redirect).toHaveBeenCalledWith(
218+
"/auth/login?returnTo=%2Fpath%2Fwith%20spaces%20%26%20special%3Dchars"
219+
);
220+
});
221+
222+
it("should URL encode returnTo from function to prevent OAuth param injection",async()=>{
223+
constwithPageAuthRequired=appRouteHandlerFactory(
224+
newAuth0Client({
225+
domain: constants.domain,
226+
clientId: constants.clientId,
227+
clientSecret: constants.clientSecret,
228+
appBaseUrl: constants.appBaseUrl,
229+
secret: constants.secret
230+
}),
231+
{
232+
loginUrl: "/auth/login"
233+
}
234+
);
235+
consthandler=withPageAuthRequired(
236+
()=>Promise.resolve(React.createElement("div",{},"foo")),
237+
{
238+
asyncreturnTo({ params }: any){
239+
return`/callback/${(awaitparams).id}?malicious=scope%3Dopenid`;
240+
}
241+
}
242+
);
243+
awaitexpect(
244+
handler({
245+
params: Promise.resolve({id: "123"})
246+
})
247+
).rejects.toThrowError("NEXT_REDIRECT");
248+
expect(redirect).toHaveBeenCalledTimes(1);
249+
expect(redirect).toHaveBeenCalledWith(
250+
"/auth/login?returnTo=%2Fcallback%2F123%3Fmalicious%3Dscope%253Dopenid"
251+
);
252+
});
168253
});
169254

170255
describe("pages router",()=>{

‎src/server/helpers/with-page-auth-required.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ export const appRouteHandlerFactory =
196196
: opts.returnTo;
197197
const{ redirect }=awaitimport("next/navigation.js");
198198
redirect(
199-
`${config.loginUrl}${opts.returnTo ? `?returnTo=${returnTo}` : ""}`
199+
`${config.loginUrl}${returnTo ? `?returnTo=${encodeURIComponent(returnTo)}` : ""}`
200200
);
201201
}
202202
returnhandler(params);

0 commit comments

Comments
(0)