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

SanitizeString: Allow hypen char #6713

Merged
merged 1 commit into from Sep 29, 2015

Conversation

Projects
None yet
3 participants
@MarcoFalke
Member

MarcoFalke commented Sep 23, 2015

Maybe someone wants to put their-domain.org in the UA?

Ref: #4983.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 23, 2015

Member

Maybe we could reverse the approach, and allow everything allowed by the BIP14. Otherwise we can keep creating 'also allow XX' pulls (yes, I'm guilty of this one) .

I think the conceptual issue is that SanitizeString is used for two purposes:

  • to avoid formatting strings that are dangerous to pass to the shell, terminal or debug log, and
  • to filter characters for user agent

Splitting of these two use-cases with a parameter was a great idea. Now we can make SanitizeString(SAFE_CHARS_UA_COMMENT) pass through all characters allowed per BIP14.

Unfortunately that BIP isn't clear on that. It says that / : ( and ) are reserved, but it doesn't make any statement about e.g. which character set is used, or even about control characters...

Member

laanwj commented Sep 23, 2015

Maybe we could reverse the approach, and allow everything allowed by the BIP14. Otherwise we can keep creating 'also allow XX' pulls (yes, I'm guilty of this one) .

I think the conceptual issue is that SanitizeString is used for two purposes:

  • to avoid formatting strings that are dangerous to pass to the shell, terminal or debug log, and
  • to filter characters for user agent

Splitting of these two use-cases with a parameter was a great idea. Now we can make SanitizeString(SAFE_CHARS_UA_COMMENT) pass through all characters allowed per BIP14.

Unfortunately that BIP isn't clear on that. It says that / : ( and ) are reserved, but it doesn't make any statement about e.g. which character set is used, or even about control characters...

@laanwj laanwj added the Feature label Sep 23, 2015

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 23, 2015

Member

Assuming ASCII and forgetting about all non printable ASCII chars, leaves us with 33 chars (alphanum is fine) to make a decision about.

.,;_/:?@() is already in, so what is left:

 !"#$%&'*+-<=>[]\^`{|}~

IIrc, UAs get dumped to debug.log as well, so is it save to allow any char?

Member

MarcoFalke commented Sep 23, 2015

Assuming ASCII and forgetting about all non printable ASCII chars, leaves us with 33 chars (alphanum is fine) to make a decision about.

.,;_/:?@() is already in, so what is left:

 !"#$%&'*+-<=>[]\^`{|}~

IIrc, UAs get dumped to debug.log as well, so is it save to allow any char?

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 23, 2015

Contributor

ut ACK

Contributor

jgarzik commented Sep 23, 2015

ut ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 23, 2015

Member

IIrc, UAs get dumped to debug.log as well, so is it save to allow any char?

Yes, but only processed through SanitizeString(SAFE_CHARS_DEFAULT) - in principle, just the fact that we don't log some characters doesn't mean that they shouldn't be allowed in uacomments.

Member

laanwj commented Sep 23, 2015

IIrc, UAs get dumped to debug.log as well, so is it save to allow any char?

Yes, but only processed through SanitizeString(SAFE_CHARS_DEFAULT) - in principle, just the fact that we don't log some characters doesn't mean that they shouldn't be allowed in uacomments.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 29, 2015

Member

Meh, I don't think it's worth it to take this to BIP level - although the BIP should have mentioned valid/invalid characters! Good to check for in next proposal if anything relating strings.
utACK

Member

laanwj commented Sep 29, 2015

Meh, I don't think it's worth it to take this to BIP level - although the BIP should have mentioned valid/invalid characters! Good to check for in next proposal if anything relating strings.
utACK

@laanwj laanwj merged commit 43edd51 into bitcoin:master Sep 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Sep 29, 2015

Merge pull request #6713
43edd51 SanitizeString: Allow hypen char (MarcoFalke)

@MarcoFalke MarcoFalke deleted the MarcoFalke:MarcoFalke-2015-allowMoreChars branch Oct 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment