Buggy app

[berkeli]

Notes available in Notes file

Bugs and issues fixed in this PR:

1. BUG #1 Inactive users should not be able to authorize
2. BUG #2 Users should not be able to access notes that they do not own.
3. BUG #3 Tags are not extracted properly when content begins with hashtag
4. BUG #4 GET /1/my/note/ enpoint is mibehaving when no ID or invalid ID is provided
5. Performance #1 Refactor GetNotesForOwner query so filtering happens in the Database and not in memory
6. [Performance N2] Refactor gRPC client tests so the wait isn't that long (was taking ~5s due to various time.After)

Backfill tests:

• Notes Model
• Utils

[berkeli]

BUG report 1

• Title
  Inactive users should not be able to authorize

• Steps to reproduce
  Create a new inactive user using the command go run ./cmd/test user -password banana -status inactive -n 1. Once user is created, try to access this users notes via curl or postman.

• Current behaviour
  The api returns notes of the user, or if user doesn't have notes it will return an empty array

• Expected behaviour
  The api should return a status of 400 with a message "Unauthorized"

• Relevant logs and/or screenshots (optional)

❯ go run ./cmd/test user -password banana -status inactive -n 1
2022/11/10 17:14:31 new user created
2022/11/10 17:14:31     id: oCLLrg1Y
2022/11/10 17:14:31     status: inactive
2022/11/10 17:14:31     password: banana
2022/11/10 17:14:31 base64 for auth: b0NMTHJnMVk6YmFuYW5h
❯ curl 127.0.0.1:8090/1/my/notes.json \
        -H 'Authorization: Basic b0NMTHJnMVk6YmFuYW5h' -i
HTTP/1.1 200 OK
Content-Type: text/json
Date: Thu, 10 Nov 2022 17:14:50 GMT
Content-Length: 12

{"notes":[]}%                                                                                                                                                 
❯ go run ./cmd/test note -owner oCLLrg1Y -n 2
2022/11/10 17:15:29 new note created
2022/11/10 17:15:29     id: c_KIcBQB
2022/11/10 17:15:29     owner: oCLLrg1Y
2022/11/10 17:15:29     content: "Example note content"
2022/11/10 17:15:29 new note created
2022/11/10 17:15:29     id: T7ev1eFF
2022/11/10 17:15:29     owner: oCLLrg1Y
2022/11/10 17:15:29     content: "Example note content"
❯ curl 127.0.0.1:8090/1/my/notes.json \
        -H 'Authorization: Basic b0NMTHJnMVk6YmFuYW5h' -i
HTTP/1.1 200 OK
Content-Type: text/json
Date: Thu, 10 Nov 2022 17:15:34 GMT
Content-Length: 333

{"notes":[{"id":"c_KIcBQB","owner":"oCLLrg1Y","content":"Example note content","created":"2022-11-10T17:15:29.328178Z","modified":"2022-11-10T17:15:29.328178Z","tags":[]},{"id":"T7ev1eFF","owner":"oCLLrg1Y","content":"Example note content","created":"2022-11-10T17:15:29.352682Z","modified":"2022-11-10T17:15:29.352682Z","tags":[]}]}% 

[berkeli]

BUG report 2

• Title
  Users should not be able to view other users notes

• Steps to reproduce

  1. Create 2 new users using the command go run ./cmd/test user -password banana -n 2. We will call them USER1 and USER2 as this will be needed.
  2. Once users are created, creates notes for USER1 with go run ./cmd/test note -owner <ID of USER1> -n 2. This will generate new notes and give IDs.
  3. Access newly created notes with USER2's access details via curl or postman. Curl command: curl 127.0.0.1:8090/1/my/note/<Note ID> -H 'Authorization: Basic <USER2 auth token>'

• Current behaviour
  The api returns notes if they exist in the database

• Expected behaviour
  The api should return a status of 400 with a message "Unauthorized"

• Relevant logs and/or screenshots (optional)

❯ go run ./cmd/test user -password banana -n 2
2022/11/10 17:25:33 new user created
2022/11/10 17:25:33     id: C5ytLjEp
2022/11/10 17:25:33     status: active
2022/11/10 17:25:33     password: banana
2022/11/10 17:25:33 base64 for auth: QzV5dExqRXA6YmFuYW5h
2022/11/10 17:25:33 new user created
2022/11/10 17:25:33     id: K8CllZ5R
2022/11/10 17:25:33     status: active
2022/11/10 17:25:33     password: banana
2022/11/10 17:25:33 base64 for auth: SzhDbGxaNVI6YmFuYW5h
❯ go run ./cmd/test note -owner C5ytLjEp -n 2
2022/11/10 17:25:58 new note created
2022/11/10 17:25:58     id: zPrt-ml9
2022/11/10 17:25:58     owner: C5ytLjEp
2022/11/10 17:25:58     content: "Example note content"
2022/11/10 17:25:58 new note created
2022/11/10 17:25:58     id: ya5dTUbo
2022/11/10 17:25:58     owner: C5ytLjEp
2022/11/10 17:25:58     content: "Example note content"
❯ curl 127.0.0.1:8090/1/my/note/zPrt-ml9 \
        -H 'Authorization: Basic eThod0ZJWHU6YXBwbGU=' -i
HTTP/1.1 200 OK
Content-Type: text/json
Date: Thu, 10 Nov 2022 17:26:27 GMT
Content-Length: 169

{"note":{"id":"zPrt-ml9","owner":"C5ytLjEp","content":"Example note content","created":"2022-11-10T17:25:58.727474Z","modified":"2022-11-10T17:25:58.727474Z","tags":[]}}%                                                                                                                                                  
❯ curl 127.0.0.1:8090/1/my/note/ya5dTUbo \
        -H 'Authorization: Basic SzhDbGxaNVI6YmFuYW5h' -i
HTTP/1.1 200 OK
Content-Type: text/json
Date: Thu, 10 Nov 2022 17:27:05 GMT
Content-Length: 169

{"note":{"id":"ya5dTUbo","owner":"C5ytLjEp","content":"Example note content","created":"2022-11-10T17:25:58.741426Z","modified":"2022-11-10T17:25:58.741426Z","tags":[]}}%

[berkeli]

BUG report 3

• Title
  Note tags are not parsed correctly. Parsing doesn't stop at space.

• Steps to reproduce

  1. Create a user using the command go run ./cmd/test user -password banana -n 1. This is not needed if you already have a user in the system.
  2. Create a note for this user with content #starts with hashtag with command: go run ./cmd/test note -owner <USER ID> -content "#starts with hashtag"
  3. Access newly created note via curl or postman

• Current behaviour
  The api returns invalid tag: "tags":["starts with hashtag"]

• Expected behaviour
  The api should return only the first word as a tag: "tags":["starts"]

• Relevant logs and/or screenshots (optional)

❯ go run ./cmd/test user -password banana -n 1
2022/11/10 17:32:04 new user created
2022/11/10 17:32:04     id: BX8XwVvL
2022/11/10 17:32:04     status: active
2022/11/10 17:32:04     password: banana
2022/11/10 17:32:04 base64 for auth: Qlg4WHdWdkw6YmFuYW5h
❯ go run ./cmd/test note -owner BX8XwVvL -content "#starts with hashtag"
2022/11/10 17:32:46 new note created
2022/11/10 17:32:46     id: 41S8Omgf
2022/11/10 17:32:46     owner: BX8XwVvL
2022/11/10 17:32:46     content: "#starts with hashtag"
❯ curl 127.0.0.1:8090/1/my/note/41S8Omgf \
        -H 'Authorization: Basic Qlg4WHdWdkw6YmFuYW5h' -i
HTTP/1.1 200 OK
Content-Type: text/json
Date: Thu, 10 Nov 2022 17:33:35 GMT
Content-Length: 190

{"note":{"id":"41S8Omgf","owner":"BX8XwVvL","content":"#starts with hashtag","created":"2022-11-10T17:32:46.895634Z","modified":"2022-11-10T17:32:46.895634Z","tags":["starts with hashtag"]}}%  

[berkeli]

BUG report 4

• Title
  GET /1/my/note/ endpoint is returning malformed response.

• Steps to reproduce

  1. Make a GET request to /1/my/note/ or /1/my/note or /1/my/note/<invalid ID> via curl or postman.

• Current behaviour
  The api returns 500 Internal server error alongside an empty note json.

• Expected behaviour
  The api should return:

  • 404 Not Found if endpoint doesn't exist, e.g. /1/my/note or /1/my/note/<invalid ID>
  • 400 Bad Request if note ID parameter isn't passed, e.g. /1/my/note/

• Relevant logs and/or screenshots (optional)
  Invalid note ID:
  
  No Note ID:
  
  /1/my/note endpoint:
  

[illicitonion]

Great bug hunting, and a really good write up - good job!

BUG report 3

• Title
  Note tags are not parsed correctly if content begins with hashtag

I think this is more around "parsing doesn't stop at space characters" rather than "if content begins with hashtag"... The example is a good one, but the problem feels more general than what's described in this issue report.

[illicitonion]

This is an interesting trade-off - the down-side of saying "no permission" rather than "note not found" is that we leak information to an attacker about what notes do exist. In general I'd say the important thing is that you handle the "not found" and "no permissions" case the same - use the same status code for both, and give a message along the lines of "Either the note didn't exist, or you didn't have permission to delete it."

This is slightly less important when handling random IDs, but becomes more important when you could be leaking personal data (e.g. whether a particular email address is signed up to your service).

[berkeli]

Yes I did think of leaking information, we definitely do not want to confirm/deny existence of anything that's sensitive.

I like the idea of giving a vague message, I'll go with that one.

[berkeli]

BUG report 5

• Title
  Stale data in cache

• Steps to reproduce
  N/A - technical

• Current behaviour
  We have an in-memory caching solution with simple get/set methods. The solution at the moment doesn't have any implementation for TTL to get rid of stale data.

• Expected behaviour
  The caching should expire old values on a schedule and should not store stale data.

[berkeli]

BUG report 6

• Title
  Get my notes should have pagination

• Steps to reproduce

  1. Create a user and create 100s of notes for that user
  2. Make a request to GET /1/my/notes.json

• Current behaviour
  This returns all notes for user without any limit. If user has 1000s of notes, this will overload the server and cause OOM killer to terminate the app

• Expected behaviour
  The api should implement database query level pagination:

  1. It(the endpoint) should accept page, per_page params in request header
  2. It should return no more records than per_page param
  3. It should return the current page, per_page and total either in the header of the response or in enveloped data
  4. It should implement sorting so the pagination works correctly

[berkeli]

BUG report 7

• Title
  Endpoints should be rate-limited

• Steps to reproduce
  Run script provided go run cmd/chaos_monkey http which will crash the server.

• Current behaviour
  Currently there's no rate limiting for endpoints and it's easy to overload the server and is prone to attacks.

• Expected behaviour
  The api should implement rate limiting so if a user is making more than X requests, they should be automatically rejected without processing.

[berkeli]

BUG report 8

• Title
  Users able to access some other users notes due to basicauth design

• Steps to reproduce
  Create 2 users:

  1. ID: test Password: test:banana
  2. ID: test:test Password: banana
     If you make requests from both users, you will only get notes for user with ID test due to the way basicAuth parses usernames & passwords.
     Basic auth for users:

1. test:test:banana
2. test:test:banana (username should be test:test, but it will be parsed as test)

• Current behaviour
  This is an edge case, but is still possible. In the example provided above, user with ID test:test will be able to see notes of user with ID test

• Expected behaviour
  There should be a mechanism that will detect such edge cases and reject the request.