Requesting source of a non-existing brews returns 500 Internal Sever Error

[AlexeySachkov]

So, here is the link to a brew source: source/msiongBI9oXz, it is accessible and returns 200 OK/304 OK.

However, if I start modifying URL to request a source of a non-existing brew, I will get unexpected 500 Internal Server Error instead of 404 Not Found:

If I remove one symbol: source/msiongBI9oX, then the status code is 500, not 404, but at least the response contains "Can not find brew".

If I add one symbol: source/msiongBI9oXz1, the status code is also 500 instead of 404, but server responds with a backtrace:

{"stack":"Error: File not found: m.\n    at Gaxios._request (/app/node_modules/gaxios/build/src/gaxios.js:129:23)\n    at runMicrotasks (<anonymous>)\n    at processTicksAndRejections (node:internal/process/task_queues:96:5)\n    at async Object.readFileMetadata (/app/server/googleActions.js:259:15)\n    at async /app/server.js:24:10\n    at async /app/server.js:170:15","message":"File not found: m.","response":{"config":{"url":"https://www.googleapis.com/drive/v3/files/m?fields=properties%2C%20createdTime%2C%20modifiedTime%2C%20description%2C%20trashed&key=AIzaSyBShL6LLvJPm-tiZm_ffds_SzoaCgdoA4g","method":"GET","userAgentDirectives":[{"product":"google-api-nodejs-client","version":"5.0.2","comment":"gzip"}],"headers":{"x-goog-api-client":"gdcl/5.0.2 gl-node/16.11.1 auth/7.1.1","Accept-Encoding":"gzip","User-Agent":"google-api-nodejs-client/5.0.2 (gzip)","Accept":"application/json"},"params":{"fields":"properties, createdTime, modifiedTime, description, trashed","key":"AIzaSyBShL6LLvJPm-tiZm_ffds_SzoaCgdoA4g"},"retry":true,"responseType":"json","retryConfig":{"currentRetryAttempt":0,"retry":3,"httpMethodsToRetry":["GET","HEAD","PUT","OPTIONS","DELETE"],"noResponseRetries":2,"statusCodesToRetry":[[100,199],[429,429],[500,599]]}},"data":{"error":{"errors":[{"domain":"global","reason":"notFound","message":"File not found: m.","locationType":"parameter","location":"fileId"}],"code":404,"message":"File not found: m."}},"headers":{"alt-svc":"h3=\":443\"; ma=2592000,h3-29=\":443\"; ma=2592000,h3-Q050=\":443\"; ma=2592000,h3-Q046=\":443\"; ma=2592000,h3-Q043=\":443\"; ma=2592000,quic=\":443\"; ma=2592000; v=\"46,43\"","cache-control":"private, max-age=0","connection":"close","content-encoding":"gzip","content-security-policy":"frame-ancestors 'self'","content-type":"application/json; charset=UTF-8","date":"Sun, 23 Jan 2022 20:03:40 GMT","expires":"Sun, 23 Jan 2022 20:03:40 GMT","server":"GSE","transfer-encoding":"chunked","vary":"Origin, X-Origin","x-content-type-options":"nosniff","x-frame-options":"SAMEORIGIN","x-xss-protection":"1; mode=block"},"status":404,"statusText":"Not Found","request":{"responseURL":"https://www.googleapis.com/drive/v3/files/m?fields=properties%2C%20createdTime%2C%20modifiedTime%2C%20description%2C%20trashed&key=AIzaSyBShL6LLvJPm-tiZm_ffds_SzoaCgdoA4g"}},"config":{"url":"https://www.googleapis.com/drive/v3/files/m?fields=properties%2C%20createdTime%2C%20modifiedTime%2C%20description%2C%20trashed&key=AIzaSyBShL6LLvJPm-tiZm_ffds_SzoaCgdoA4g","method":"GET","userAgentDirectives":[{"product":"google-api-nodejs-client","version":"5.0.2","comment":"gzip"}],"headers":{"x-goog-api-client":"gdcl/5.0.2 gl-node/16.11.1 auth/7.1.1","Accept-Encoding":"gzip","User-Agent":"google-api-nodejs-client/5.0.2 (gzip)","Accept":"application/json"},"params":{"fields":"properties, createdTime, modifiedTime, description, trashed","key":"AIzaSyBShL6LLvJPm-tiZm_ffds_SzoaCgdoA4g"},"retry":true,"responseType":"json","retryConfig":{"currentRetryAttempt":0,"retry":3,"httpMethodsToRetry":["GET","HEAD","PUT","OPTIONS","DELETE"],"noResponseRetries":2,"statusCodesToRetry":[[100,199],[429,429],[500,599]]}},"code":404,"errors":[{"domain":"global","reason":"notFound","message":"File not found: m.","locationType":"parameter","location":"fileId"}]}

[AlexeySachkov]

On top of that, accessing source/ route seems to return the Home page, but without the welcome message

[G-Ambatte]

This appears to be the line that is at least in part causing the issue; any ID over 12 characters long is treated as a Google brew ID. This is then passed here, where the Google ID parameter is not validated before an attempt is made to retrieve the Google Brew using that ID.

So it should be possible to:

1. validate the Google Access ID parameter (even a basic accessID.length == 33 would be an improvement), and
2. throw an error when validation fails, to be handled by the calling function.

[calculuschild]

validate the Google Access ID parameter (even a basic accessID.length == 33 would be an improvement)

The reason we did >12 is because Google IDs are not guaranteed to be a particular length. Older files (from 10+ years ago) have fewer characters, current files have 33 or whatever the number is, but every so often the number is incremented when security protocols change, etc. At a minimum we can assume they won't be getting shorter, but I wouldn't lock in a fixed value.

Unfortunately I don't think we can have a 100% surefire way to pre-validate a Google File ID by character count alone. Hence, the current behavior of just letting Google validate the file id for us.

[ericscheid]

Eventually we'll have stubs in mongo for brews, including what the google id should be.

We can then verify if the >12mumble is a known google id and respond appropriately.

What "appropriately" actually is .. is not yet defined...

• if mongo barfed → error 500
• if 12 chars && not found in mongo → error 404 Not Found
• if 12 chars && found in mongo but marked as deleted → error 410 Gone
• if 12 chars && found in mongo but marked as blocked due to DMCA → error 451 Unavailable For Legal Reasons
• if 12 chars && found in mongo and all else good → result 200 OK
• if >12 chars && not found in mongo.googID → error 404 Not Found
• if >12 chars && found in mongo.googID but API barfed → error 500
• if >12 chars && found in mongo.googID but not found via drive API → error 404 Not Found
• etc

[ericscheid]

• if server is having an existential identity crisis → 418 I'm a teapot

[calculuschild]

if >12 chars && not found in mongo.googID → error 404 Not Found

Unless that brew hasn't yet been visited since stubs were added, in which case we have no record of it yet. We can also never guarantee that a given Google brew has an associated stub.

[ericscheid]

Unless that brew hasn't yet been visited since stubs were added, in which case we have no record of it yet. We can also never guarantee that a given Google brew has an associated stub.

Good point.

(We can also make stubs when that user visit's their user page .. we can see HB knows about drive brews because it lists them).

Hmm...

• if >12 chars && not found in mongo.googID → might be unmet googleID, so attempt a google API call ...
  • if API gives 404, then not a valid googleID (but might be if the user is moving files around) → future requests should be allowed to try again (thus, do not blacklist)
  • if API gives 200, then is valid googleID (for now, might change in future if file is moved to trash) → cache into mongo (pointless for this is-valid test, but useful for the other reasons we're making stubs)

⇒ yep, we can never be certain that an unknown >12string is an actual google brew or not without attempting an API call.

---

Oh hey, what actually happens when a google brew is deleted? Is the file on Drive simply moved to trash, where the user could retrieve it? Or is it obliterated?

[tappity-tap-tap-clickety-click]

Ah, despite the ominous double-confirmation warnings about permanent obliteration, the deleted goog-brew is instead moved to the drive-bin, where it can then be retrieved. Interesting. Complicating (w.r.t. potential mongo stub isDeleted flag). Additional complications when a brew is moved from drive to mongo (the drive version is moved to /drive/bin). Updates would get saved into mongo (of course).

...

This..

What "appropriately" actually is .. is not yet defined...

.. is an understatement =)

[ericscheid]

We now have stubs, so what happens?

If I remove one symbol: source/msiongBI9oX, then the status code is 500, not 404, but at least the response contains "Can not find brew".

If I add one symbol: source/msiongBI9oXz1, the status code is also 500 instead of 404, but server responds with a backtrace:

Both still true

[calculuschild]

At <= 12 characters, it assumes a stub/local brew. At >12 characters, it assumes a Google Drive (unstubbed) file. In either case we should implement a better error page.

[5e-Cleric]

Basic ErrorPage Functionality #2892 is related.

[G-Ambatte]

If I remove one symbol: source/msiongBI9oX, then the status code is 500, not 404, but at least the response contains "Can not find brew".

https://github.com/naturalcrit/homebrewery/blob/69ef4d765312d4d125538b6f4adb2e85e16bf2c6/server/app.js#L465

I suspect what is/was happening is that the Google error was throwing the error with the 404 in err.code but Homebrewery was checking against err.status. So Google errors (for missing files, at least) resulted in the default of 500, In the previously mentioned PR #2892, I have changed this line to const status = err.status || err.code || 500; which to pass the 404 correctly; of course, this is immediately handled and redirected to the new Error Page created in that PR.

[G-Ambatte]

As #2892 is now completed and merged, we should be able to close this issue. Specifically, attempting to load the /source/ route of a non-existent brew results in a 404.