OAK-12259: oak-http: fix HTTP Basic credential parsing in OakServlet#2957
OAK-12259: oak-http: fix HTTP Basic credential parsing in OakServlet#2957ciechanowiec wants to merge 1 commit into
Conversation
Split the decoded Authorization header on the first colon only (RFC 7617) so passwords containing colons are preserved instead of being silently truncated, and reject missing/malformed headers with a LoginException (mapped to HTTP 401) instead of throwing an ArrayIndexOutOfBoundsException (HTTP 500). Adds OakServletTest covering the parsing behaviour.
| private static void assertUnauthorized(String authorization) | ||
| throws Exception { | ||
| ContentRepository repository = mock(ContentRepository.class); | ||
| HttpServletRequest request = mock(HttpServletRequest.class); | ||
| when(request.getHeader("Authorization")).thenReturn(authorization); | ||
| HttpServletResponse response = mock(HttpServletResponse.class); | ||
|
|
||
| new OakServlet(repository).service(request, response); | ||
|
|
||
| verify(response).setHeader("WWW-Authenticate", "Basic realm=\"Oak\""); | ||
| verify(response).sendError(HttpServletResponse.SC_UNAUTHORIZED); | ||
| verify(repository, never()).login(any(), any()); | ||
| } |
There was a problem hiding this comment.
Nits : Move the helper method to the end of Test file.
| if (colon < 0) { | ||
| throw new LoginException("Malformed Basic credentials: missing ':' separator"); | ||
| } | ||
| String userId = decoded.substring(0, colon); |
There was a problem hiding this comment.
Shouldn't we explicitly check for whether the username has a : or not, and throw an exception if it has ?
|
If we are indeed following https://www.rfc-editor.org/info/rfc7617/, we should document that. @Amoratinos @anchela wdyt ? |
|
While at it, we should also handle the whitespace after "Basic" correctly (https://www.greenbytes.de/tech/specs/rfc7235.html#challenge.and.response), case sensitivity, decoding non-ASCII, invalid username/passwords. For obvious reasons, I'll take this. |
reschke
left a comment
reschke
left a comment
There was a problem hiding this comment.
While at it, we should also handle the whitespace after "Basic" correctly (https://www.greenbytes.de/tech/specs/rfc7235.html#challenge.and.response), case sensitivity, decoding non-ASCII, invalid username/passwords.
For obvious reasons, I'll take this.
|
@ciechanowiec - idea:
Do you want to give that a try, or should I take over (of course acking you)? |
|
@reschke , feel free to take over. I doubt I'll have the time/capacity to work on this issue in the foreseeable future. |
3 participants
Fixes OAK-12259.