Skip to content

[Recorder] Fix encode method for special characters - ~#17462

Closed
HarshaNalluru wants to merge 4 commits intoAzure:mainfrom
HarshaNalluru:joheredi-branch
Closed

[Recorder] Fix encode method for special characters - ~#17462
HarshaNalluru wants to merge 4 commits intoAzure:mainfrom
HarshaNalluru:joheredi-branch

Conversation

@HarshaNalluru
Copy link
Copy Markdown
Contributor

@HarshaNalluru HarshaNalluru commented Sep 4, 2021

@joheredi reached out with a problem where the playback was failing upon rerecording a certain test.

Discussed this with @sadasant who has originally implemented the encoding/decoding parts.
The reason being the generated recording was not replaced as per the replaceable variables provided.
The reason being the secret had ~ character, and encodeRFC3986 method doesn't encode the character.
RFC 3986 standard does not encode the character, but the recordings have that encoded, so adding a new method encodeSpecialCharacters in the recorder to encode ~ character to allow the replacements to happen smoothly.

In case we find such characters in future, they should be added to the list in encodeSpecialCharacters method that is being added in this PR.

@HarshaNalluru HarshaNalluru marked this pull request as ready for review September 4, 2021 00:03
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the record, RFC 3986 says:

Unreserved Characters

Characters that are allowed in a URI but do not have a reserved
purpose are called unreserved. These include uppercase and lowercase
letters, decimal digits, hyphen, period, underscore, and tilde.

unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"

Later it says:

When a URI is dereferenced, the components and subcomponents
significant to the scheme-specific dereferencing process (if any)
must be parsed and separated before the percent-encoded octets within
those components can be safely decoded, as otherwise the data may be
mistaken for component delimiters. The only exception is for
percent-encoded octets corresponding to characters in the unreserved
set, which can be decoded at any time. For example, the octet
corresponding to the tilde ("~") character is often encoded as "%7E"
by older URI processing implementations; the "%7E" can be replaced by
"~" without changing its interpretation.

This specification was added in 2005. JS encoding methods are likely older (but there could be some other explanation of why this is missing that I might not be aware of).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

since the specification says that these are the characters they ignore: "-" / "." / "_" / "~",

let’s make sure we have a test in which we check how bad things get if we have a secret with those other characters 🙌

@deyaaeldeen
Copy link
Copy Markdown
Member

deyaaeldeen commented Sep 24, 2021

FYI I bumped into the encoded tilde issue in #17732 and I had to replace the client secret manually to get unblocked. I also confirm that I tested this branch and it works great in my case!

@azure-sdk
Copy link
Copy Markdown
Collaborator

API changes have been detected in @azure-rest/purview-catalog. You can review API changes here

@azure-sdk
Copy link
Copy Markdown
Collaborator

API changes have been detected in @azure-rest/core-client. You can review API changes here

API changes

+     binaryContent?: boolean;

@ramya-rao-a
Copy link
Copy Markdown
Contributor

Am I missing something here? The PR talks about recorder issue, but the files updated include the confidential-ledger, purview and core packages with changes unrelated to the recorder...

@HarshaNalluru HarshaNalluru marked this pull request as draft November 22, 2021 19:46
@HarshaNalluru
Copy link
Copy Markdown
Contributor Author

Testing the fix on Jose's branch.
Moved to draft. :)

@ramya-rao-a
Copy link
Copy Markdown
Contributor

@joheredi, @HarshaNalluru, what are the next steps here?

@joheredi
Copy link
Copy Markdown
Member

joheredi commented Dec 7, 2021

@joheredi, @HarshaNalluru, what are the next steps here?

I believe the new recorder wouldn't run into these issues, so maybe the best plan forward would be to migrate the libraries impacted by this.

@ramya-rao-a
Copy link
Copy Markdown
Contributor

@HarshaNalluru In that case, I would recommend closing any ongoing PRs for the old recorder

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.

6 participants