Skip to content
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

fix the % decoding error #43

Closed
wants to merge 2 commits into from
Closed

fix the % decoding error #43

wants to merge 2 commits into from

Conversation

alextremp
Copy link

Description

This PR fixes the #30 Issue.

Before:

  • when parsing a query string containing a %, p.ex.: 'ka=va&kb=vb&kc&kd=%vd', the '%vd' decoding fails:

image

After:

  • encoding the received data first, causes reserved chars to be valid for next decoding operation when the data is splitted and parsed separately:

image

Further considerations

  • Current test system failed to me, so I've tested the solution with custom test code using mocha+chai for the test case. To reproduce it:
npm install mocha chai --save-dev
// ./test/indexTest.js

const expect = require('chai').expect

describe('querystring', () => {
  const qs = require('../index')
  describe('parse', () => {
    it('should parse query string key-value pairs', () => {
      const givenQS = 'ka=va&kb=vb'
      const expected = {
        ka: 'va',
        kb: 'vb'
      }
      const result = qs.parse(givenQS)
      expect(result).to.deep.equal(expected)
    })
    it('should parse query string key-only values', () => {
      const givenQS = 'ka=va&kb=vb&kc'
      const expected = {
        ka: 'va',
        kb: 'vb',
        kc: ''
      }
      const result = qs.parse(givenQS)
      expect(result).to.deep.equal(expected)
    })
    it('should parse query string with reserved chars', () => {
      const givenQS = 'ka=va&kb=vb&kc&kd=%vd'
      const expected = {
        ka: 'va',
        kb: 'vb',
        kc: '',
        kd: '%vd'
      }
      const result = qs.parse(givenQS)
      expect(result).to.deep.equal(expected)
    })
  })
})

@SidneyNemzer
Copy link

Will this prevent decoding valid percent-encoded values?

example.com?test=test%20test
// => { test: 'test test' }

@alextremp
Copy link
Author

good point @SidneyNemzer
Yes, the change was preventing valid percent-encoded values (the original tests fail on command, so didn't test that 🤦‍♂)

Updated:

  • instead of encoding the QS first, the change of this PR just adds a "safeDecodeURIComponent" that returns the decoded value or the original value if the decode fails.
  • also, added a test for the commented case

Tx!

@alextremp
Copy link
Author

closing due to no maintainers of the project, and stopped using it

@alextremp alextremp closed this Feb 11, 2020
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.

2 participants