Skip to content

Integrate basic auth #63

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

Closed
wants to merge 3 commits into from

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Nov 12, 2021

As mentioned in #57 it could be a security issue, that the basic-auth package could be potentially hijacked.

This PR integrates basic-auth npm package into node-oauth2-server.

I checked with safeRegex if the regexes are safe from catastrophic backtracking.

I removed safe-Buffer as dependency, as we are supporting modern node versions anyway. Also reducing potential attack vector by a hijacked safe-Buffer-package.

Unit tests were integrated also.

Also added lint:fix as new script... was easier than adding the missing semicolons by hand. lol

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Nov 12, 2021

What should I do about the false credential errors?

@jankapunkt
Copy link
Member

@Uzlopak before you continue to work on this, let's first come to a conclusion in #57 I am checking in the meantime if we can make the tests more dynamic to avoid using hard-coded values.

@jwerre
Copy link
Contributor

jwerre commented Nov 12, 2021

Also, please move to lib/utils. I know you just integrated basic-auth but it's better to return errors than to throw them

I wonder if something like this would be better. But again, it would be a major version bump:

module.exports = ( function() {

  const CREDENTIALS_REGEXP = /^ *(?:[Bb][Aa][Ss][Ii][Cc]) +([A-Za-z0-9._~+/-]+=*) *$/;
  const USER_PASS_REGEXP = /^([^:]*):(.*)$/;

  function Credentails(username, password) {
    this.username = username;
    this.password = password;
  }

  let instance;

  return {
    parse: function(value) {

      if (typeof value !== 'string') {
        return undefined;
      }

      // parse header
      const match = CREDENTIALS_REGEXP.exec(value);
      if (!match || !match.length) {
        return undefined;
      }

      const result = USER_PASS_REGEXP.exec(Buffer.from(match[1], 'base64').toString());
      if (!result || !result.length) {
        return undefined;
      }

      if (instance == null) {
        instance = new Credentails(result[1], result[2]);
        instance.constructor = null;
      }

      return instance;
    }
  };
})();

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Nov 12, 2021

Lets discuss this in #57

@Uzlopak Uzlopak closed this Dec 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants