Skip to content

Fix bug when decoding JWT in browser #422

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 17, 2019
Merged

Fix bug when decoding JWT in browser #422

merged 2 commits into from
Jul 17, 2019

Conversation

Aschen
Copy link
Contributor

@Aschen Aschen commented Jul 16, 2019

What does this PR do?

Since the SDK is available for Node.js and for browsers, the decoding of the JWT from base64 is done either with Buffer.from(jwt, 'base64') for Node.js or atob(base64) for browsers.
The check to know the runtime was if (Buffer), and since this class is undefined in the browsers, this line throw an exception who is catched by the JSON.parse try...catch.

This PR fix the check to infer the runtime with if (typeof Buffer !== 'undefined') instead.

Fix #407

How should this be manually tested?

  • Step 1 : Create an this HTML file at the repository root and open it in your browser:
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>Kuzzle SDK Embedded</title>
    <script type="text/javascript" src="dist/kuzzle.js"></script>
  </head>
  <body>
    <button id="start" onclick="start()">Start</button>
    <script type="text/javascript">
      async function start() {
        window.kuzzle = new KuzzleSDK.Kuzzle(new KuzzleSDK.Http('localhost', { port: 7512}))
        window.kuzzle.on('networkError', () => console.log('Catched by listener'))

        try {
          await window.kuzzle.connect()

          const jwt = await window.kuzzle.auth.login('local', { username: 'admin', password: 'admin' })
          console.log(jwt)
        } catch (error) {
          console.log("Catched by try ... catch")
          console.log(error)
        }
      }

    </script>
  </body>
</html>

@Aschen Aschen self-assigned this Jul 16, 2019
@Aschen Aschen mentioned this pull request Jul 16, 2019
Closed
@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #422 into 6-dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           6-dev    #422   +/-   ##
=====================================
  Coverage   96.3%   96.3%           
=====================================
  Files         32      32           
  Lines       1517    1517           
=====================================
  Hits        1461    1461           
  Misses        56      56
Impacted Files Coverage Δ
src/core/Jwt.js 95.23% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 079d964...4e992a5. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 16, 2019 •
edited
Loading

Codecov Report

Merging #422 into 6-dev will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           6-dev     #422      +/-   ##
=========================================
+ Coverage   96.3%   96.38%   +0.07%     
=========================================
  Files         32       32              
  Lines       1517     1522       +5     
=========================================
+ Hits        1461     1467       +6     
+ Misses        56       55       -1
Impacted Files Coverage Δ
src/core/Jwt.js 100% <100%> (+4.76%) ⬆️
src/protocols/http.js 92.68% <0%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 079d964...5ccd1e0. Read the comment docs.

scottinet
scottinet suggested changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@scottinet scottinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be a unit test making sure that this error is permanently fixed

@scottinet scottinet merged commit 589b60a into 6-dev Jul 17, 2019
@scottinet scottinet deleted the 407-fix-bug-when-decoding-jwt-in-browser branch July 17, 2019 15:18
@scottinet scottinet mentioned this pull request Jul 31, 2019
Merged
scottinet added a commit that referenced this pull request Jul 31, 2019
@scottinet
Merge pull request #434 from kuzzleio/6.2.0-proposal
0b4c123 
# [6.2.0](https://github.com/kuzzleio/sdk-javascript/releases/tag/6.2.0) (2019-07-31)


#### Bug fixes

- [ [#428](#428) ] Properly handle boolean flags in HTTP querystrings   ([scottinet](https://github.com/scottinet))
- [ [#427](#427) ] Solve promise+event+memory leaks when the network fails   ([scottinet](https://github.com/scottinet))
- [ [#424](#424) ] Prevent pending request leak when disconnect the SDK   ([Aschen](https://github.com/Aschen))
- [ [#422](#422) ] Fix bug when decoding JWT in browser   ([Aschen](https://github.com/Aschen))
- [ [#420](#420) ] Fix http protocol unresolved promise on connection error   ([Aschen](https://github.com/Aschen))

#### New features

- [ [#419](#419) ] Add bulk:write and bulk:mWrite   ([Aschen](https://github.com/Aschen))

#### Enhancements

- [ [#421](#421) ] Get api routes from server:publicApi   ([Aschen](https://github.com/Aschen))
- [ [#423](#423) ] Emit queryError event on malformed request   ([Aschen](https://github.com/Aschen))
- [ [#417](#417) ] Security controller documentation   ([benoitvidis](https://github.com/benoitvidis))
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@scottinet scottinet scottinet approved these changes

@Yoann-Abbes Yoann-Abbes Yoann-Abbes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Aschen Aschen

Labels

changelog:bug-fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Aschen @scottinet @Yoann-Abbes