From string validation by smyrman · Pull Request #75 · rs/xid

smyrman · 2022-03-04T08:13:52Z

Resolves #71.

commit 06d3085 (HEAD -> from-string, smyrman/from-string)
Author: Sindre Myren sindre@clarify.io
Date: Thu Mar 10 19:39:00 2022 +0100

fix: let decode look for additional base32 padding

Update FromString and XID.TextUnmarshal so that it looks for discarded
bits in the final source character. This ensures that XIDs that have
been manually tampered with in a way that's ignored by base32 decode,
will not pass as valid.

commit f288272
Author: Sindre Myren sindre@clarify.io
Date: Fri Mar 4 09:13:06 2022 +0100

add benchmark and new failing test for FromString

commit 2171fc4
Author: Sindre Myren sindre@clarify.io
Date: Fri Mar 4 09:12:03 2022 +0100

error.go: make ErrInvalidID a const

Make ErrInvalidID a constant instead of a variable. This prevent it from
being changed by external packages; a behavior that although allowed by
the compiler, should probably be considered an invalid operation.

commit b64cbc7
Author: Sindre Myren sindre@clarify.io
Date: Fri Mar 4 09:09:13 2022 +0100

chore: address linter issues

Perform changes suggested by linter; no functional changes.

Perform changes suggested by linter; no functional changes.

Make ErrInvalidID a constant instead of a variable. This prevent it from being changed by external packages; a behavior that although allowed by the compiler, should probably be considered an invalid operation.

smyrman · 2022-03-04T08:15:01Z

go test -bench 'FromString*'
goos: darwin
goarch: amd64
pkg: github.com/rs/xid
cpu: Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
BenchmarkFromString-4    	83381887	        13.95 ns/op
BenchmarkFromString2-4   	57998780	        19.95 ns/op
PASS
ok  	github.com/rs/xid	4.594s
go test -bench 'FromString*'  16.73s user 0.52s system 278% cpu 6.191 total

smyrman · 2022-03-04T08:20:00Z

% go test -bench 'FromString*' -benchmem
goos: darwin
goarch: amd64
pkg: github.com/rs/xid
cpu: Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
BenchmarkFromString-4           76597575                14.00 ns/op            0 B/op          0 allocs/op
BenchmarkFromString2-4          59851599                20.47 ns/op            0 B/op          0 allocs/op
PASS
ok      github.com/rs/xid       3.378s
go test -bench 'FromString*' -benchmem  12.67s user 0.83s system 214% cpu 6.290 total

smyrman · 2022-03-04T09:11:16Z

Verifying that fromString2 actually solves #71:

go test -v -run 'TestFromString.*Quick.*'
=== RUN   TestFromStringQuick
    id_test.go:292: comparing XIDs:
        a: "c8gtev2sahsk17no3shg"
        b: "c8gtev2sahsk17no3shi" (index 19 changed to i)
    id_test.go:307: #1: failed on input xid.ID{0x62, 0x21, 0xd7, 0x7c, 0x5c, 0x54, 0x79, 0x40, 0x9e, 0xf8, 0x1f, 0x23}, 0x69
--- FAIL: TestFromStringQuick (0.00s)
=== RUN   TestFromString2Quick
--- PASS: TestFromString2Quick (0.00s)
=== RUN   TestFromString2QuickInvalidChars
--- PASS: TestFromString2QuickInvalidChars (0.00s)
FAIL
exit status 1
FAIL	github.com/rs/xid	0.293s
1 smyrman@patchi ~/Code/xid (git)-[from-strin

smyrman · 2022-03-04T09:12:06Z

I will clean up this PR if we decide that we want the semantics of fromString2 over FromString, and that we are happy with the performance implications.

smyrman · 2022-03-10T18:46:01Z

Updated benchmarks.

Before change

$ go test -bench '.*' -run '^$'
goos: darwin
goarch: amd64
pkg: github.com/rs/xid
cpu: Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
BenchmarkNew-4          	29175643	        40.74 ns/op
BenchmarkNewString-4    	19683991	        54.16 ns/op
BenchmarkFromString-4   	77936979	        14.54 ns/op
PASS
ok  	github.com/rs/xid	5.598s
go test -bench '.*' -run '^$'  20.10s user 0.54s system 320% cpu 6.441 total

New failing test:

$ go test
--- FAIL: TestFromStringQuick (0.00s)
    id_test.go:265: comparing XIDs:
        a: "c8l4djisahsnf5k88go0"
        b: "c8l4djisahsnf5k88goa" (index 19 changed to a)
    id_test.go:280: #1: failed on input xid.ID{0x62, 0x2a, 0x46, 0xce, 0x5c, 0x54, 0x79, 0x77, 0x96, 0x88, 0x44, 0x30}, 0x61
--- FAIL: TestFromStringQuickInvalidChars (0.00s)
    id_test.go:292: comparing XIDs:
        a: "c8l4djisahsnf5k88gvg"
        b: "c8l4djisahsnf5k88gvl" (index 19 changed to l)
    id_test.go:307: #15: failed on input xid.ID{0x62, 0x2a, 0x46, 0xce, 0x5c, 0x54, 0x79, 0x77, 0x96, 0x88, 0x44, 0x3f}, 0x6c
FAIL
exit status 1
FAIL	github.com/rs/xid	0.160s

After change

$ go test -bench '.*' -run '^$'
goos: darwin
goarch: amd64
pkg: github.com/rs/xid
cpu: Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
BenchmarkNew-4          	28826906	        40.04 ns/op
BenchmarkNewString-4    	20088812	        50.65 ns/op
BenchmarkFromString-4   	64955624	        17.20 ns/op
PASS
ok  	github.com/rs/xid	5.484s
go test -bench '.*' -run '^$'  19.65s user 0.95s system 237% cpu 8.678 total

Passing tests:

$ go test
PASS
ok  	github.com/rs/xid	0.279s

rs · 2022-03-10T18:46:25Z

LGTM. Do you need to do something more before merge?

smyrman · 2022-03-10T18:50:38Z

Do you need to do something more before merge?

Well, I have some questions.

I changed errors to be const instead of var -- this might be viewed as a breaking change (although I would argue that programs that change pacakge errors are already invalid). Is this a change that you want, or should I pull it out?
Do you want a separate error for invalid padding (currently in the PR), or should there just be one error for all invalid IDs?

rs · 2022-03-10T18:59:19Z

Const is fine and same error may be fine as well, I let you judge. I'm not sure a separate error is actionable.

smyrman · 2022-03-10T19:00:54Z

👍 I agree; I will remove the second error.

smyrman · 2022-03-10T19:07:23Z

Done & rebased.

Update FromString and XID.TextUnmarshal so that it looks for discarded bits in the final source character. This ensures that XIDs that have been manually tampered with in a way that's ignored by base32 decode, will not pass as valid.

smyrman · 2022-03-10T19:11:38Z

Fixed commit messages.

smyrman · 2022-03-10T19:12:56Z

Ready for merge 👍

smyrman added 2 commits March 4, 2022 09:09

chore: address linter issues

b64cbc7

Perform changes suggested by linter; no functional changes.

error.go: make ErrInvalidID a const

2171fc4

Make ErrInvalidID a constant instead of a variable. This prevent it from being changed by external packages; a behavior that although allowed by the compiler, should probably be considered an invalid operation.

smyrman changed the title ~~from string~~ WIP: from string validation Mar 4, 2022

smyrman mentioned this pull request Mar 4, 2022

[bug] xid.FromString return string is difference. #71

Closed

smyrman force-pushed the from-string branch 4 times, most recently from 723335c to 7868550 Compare March 4, 2022 09:09

add benchmark and new failing test for FromString

f288272

smyrman force-pushed the from-string branch from 7868550 to c49b848 Compare March 10, 2022 18:39

rs marked this pull request as ready for review March 10, 2022 18:44

smyrman changed the title ~~WIP: from string validation~~ From string validation + minor linter cleanups Mar 10, 2022

smyrman changed the title ~~From string validation + minor linter cleanups~~ From string validation Mar 10, 2022

smyrman force-pushed the from-string branch from c49b848 to 33bd2c8 Compare March 10, 2022 19:05

smyrman force-pushed the from-string branch from 33bd2c8 to a8755d3 Compare March 10, 2022 19:08

fix: let decode look for additional base32 padding

06d3085

Update FromString and XID.TextUnmarshal so that it looks for discarded bits in the final source character. This ensures that XIDs that have been manually tampered with in a way that's ignored by base32 decode, will not pass as valid.

smyrman force-pushed the from-string branch from a8755d3 to 06d3085 Compare March 10, 2022 19:11

rs merged commit 66f8c42 into rs:master Mar 10, 2022

fufuok added a commit to fufuok/xid that referenced this pull request Mar 12, 2022

perf: From string validation (rs#75) @smyrman

e89aee3

robxyy mentioned this pull request Oct 9, 2022

Just need to check the last byte. #89

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

From string validation#75

From string validation#75
rs merged 4 commits into
rs:masterfrom
smyrman:from-string

smyrman commented Mar 4, 2022 •

edited

Loading

Uh oh!

smyrman commented Mar 4, 2022 •

edited

Loading

Uh oh!

smyrman commented Mar 4, 2022

Uh oh!

smyrman commented Mar 4, 2022

Uh oh!

smyrman commented Mar 4, 2022

Uh oh!

smyrman commented Mar 10, 2022

Uh oh!

rs commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022 •

edited

Loading

Uh oh!

rs commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

smyrman commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smyrman commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smyrman commented Mar 4, 2022

Uh oh!

smyrman commented Mar 4, 2022

Uh oh!

smyrman commented Mar 4, 2022

Uh oh!

smyrman commented Mar 10, 2022

Before change

After change

Uh oh!

rs commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rs commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022

Uh oh!

smyrman commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

smyrman commented Mar 4, 2022 •

edited

Loading

smyrman commented Mar 4, 2022 •

edited

Loading

smyrman commented Mar 10, 2022 •

edited

Loading

smyrman commented Mar 10, 2022 •

edited

Loading