Dead check in UpdateAccessTokenFilter.java, possible security issue

[pieterlukasse]

Expected behavior

The jwt variable should not always be null or the check should be removed.

Actual behavior

The jwt is always null here: https://github.com/OHDSI/WebAPI/blob/723eb4cbe470dc58d1fa340add5c2d9cc89a4e28/src/main/java/org/ohdsi/webapi/shiro/filters/UpdateAccessTokenFilter.java#L124

Steps to reproduce behavior

Just reading the code is sufficient to conclude that.

I am currently making changes to this class, so it would be very useful to know what the original intention of this part was.

[pieterlukasse]

@chrisknoll , @rkboyce : maybe one of you can help me understand this part?

[pieterlukasse]

Looks like it was introduced here: https://github.com/OHDSI/WebAPI/pull/832/files#diff-2ab1eba233c07d9653dbc0312742cf31baea8a755f797445784064efa44f88f9L76-R92 by @ssuvorov-fls

[chrisknoll]

I'm not sure, but you are correct: the jwt is defaulted to null, and therefore it is never the case where jwt is not null, so that if statement seems to be redundant. This is either an oversight of some refactoring, or jwt should be initialized to something earlier in the method.

Other than the seemingly unnecessary null check, have you identified a specific issue (ie: a vulnerability of not checking for existing JWT).

[pieterlukasse]

Yeah, I'm seeing some weird behavior related to sessionId, and I think this extra change on top of the above created some circular logic: https://github.com/OHDSI/WebAPI/commit/458a2c3b3f6fbe2644c3918f32d0728d0a0e4963#diff-2ab1eba233c07d9653dbc0312742cf31baea8a755f797445784064efa44f88f9R136-R144

      if (sessionId == null) {
        final String token = TokenManager.extractToken(request);
        if (token != null) {
          sessionId = (String) TokenManager.getBody(token).get(Constants.SESSION_ID);
        }
      }

      Date expiration = this.getExpirationDate(this.tokenExpirationIntervalInSeconds);
      jwt = TokenManager.createJsonWebToken(login, sessionId, expiration);

I.e. the sessionId is expected in a field matching Constants.SESSION_ID(set to Session-ID).... a field which is only really initialized in createJsonWebToken as far as I can see, as my initial token does not have the Session-ID field.