Project

General

Profile

Actions

Bug #16774

closed

keep-web needs to return user-visible errors

Added by Peter Amstutz almost 4 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Story points:
2.0
Release relationship:
Auto

Description

I tried to download a file from keep-web with an invalid token. In the browser, without using developer tools, there is no feedback to the user why it didn't work.

  1. There is no error page. For any 4xx or 5xx error it should return a minimal error page
  2. Despite the error is actually that the token is invalid, it returns 404 instead of 401
  3. A 404 can mean either that an item doesn't exist, or that the user doesn't have permission to see it. The error text should reflect that.

Sort of related, I also realized that if keep-web sets a cookie with the API token, there's no way for the user to clear that cookie without going into browser settings.


Subtasks 1 (0 open1 closed)

Task #17027: Review 16774-keep-web-errorsResolvedNico César11/20/2020Actions

Related issues

Related to Arvados Epics - Idea #16444: Improved error detection/reportingClosed03/01/202204/30/2022Actions
Related to Arvados - Feature #16669: Accept OpenID Connect access tokenResolvedTom Clegg09/24/2020Actions
Actions #1

Updated by Peter Amstutz almost 4 years ago

  • Description updated (diff)
Actions #2

Updated by Peter Amstutz almost 4 years ago

  • Description updated (diff)
Actions #3

Updated by Peter Amstutz almost 4 years ago

  • Related to Idea #16444: Improved error detection/reporting added
Actions #4

Updated by Peter Amstutz almost 4 years ago

  • Target version changed from 2020-09-23 Sprint to 2020-10-07 Sprint
Actions #5

Updated by Peter Amstutz over 3 years ago

  • Target version changed from 2020-10-07 Sprint to 2020-10-21 Sprint
Actions #6

Updated by Peter Amstutz over 3 years ago

  • Target version deleted (2020-10-21 Sprint)
Actions #7

Updated by Peter Amstutz over 3 years ago

  • Target version set to 2020-11-04 Sprint
Actions #8

Updated by Peter Amstutz over 3 years ago

  • Subject changed from keep-web should return better errors to keep-web needs to return user-visible errors
Actions #9

Updated by Peter Amstutz over 3 years ago

  • Description updated (diff)
Actions #10

Updated by Peter Amstutz over 3 years ago

  • Description updated (diff)
Actions #11

Updated by Peter Amstutz over 3 years ago

  • Category set to Keep
Actions #12

Updated by Peter Amstutz over 3 years ago

  • Assigned To set to Peter Amstutz
Actions #13

Updated by Peter Amstutz over 3 years ago

  • Target version changed from 2020-11-04 Sprint to 2020-11-18
Actions #14

Updated by Peter Amstutz over 3 years ago

  • Target version changed from 2020-11-18 to 2020-12-02 Sprint
Actions #15

Updated by Peter Amstutz over 3 years ago

  • Story points set to 2.0
Actions #16

Updated by Tom Clegg over 3 years ago

Actions #17

Updated by Peter Amstutz over 3 years ago

16774-keep-web-errors @ 34f5aff9a166c0e03564b607981284b6d4af9548

developer-run-tests: #2191

WebDAV errors are currently plain text but maybe they should follow this https://www.ietf.org/proceedings/50/I-D/webdav-status-00.txt

S3 errors are now XML as expected by S3 clients.

Actions #18

Updated by Tom Clegg over 3 years ago

Might be worth using XML encoder here, in case error messages/paths have "<" chars etc.

Actions #19

Updated by Peter Amstutz over 3 years ago

16774-keep-web-errors @ aa1c0f3049f7b78e7590dde868b915bef9a7ebbe

Fix tests

Use xml.EncodeElement for error response

developer-run-tests: #2194

Actions #20

Updated by Nico César over 3 years ago

review at aa1c0f3049f7b78e7590dde868b915bef9a7ebbe

services/keep-web/handler.go:

  • Maybe I'm too picky here, but any reason to have "\n" in the errors? Would this cause issues on non-Unix systems that use "\r\n" like Windows? also reading RFC 2046 https://www.ietf.org/rfc/rfc2046.txt section 4.1.1 says: "The canonical form of any MIME "text" subtype MUST always represent a line break as a CRLF sequence. Similarly, any occurrence of CRLF in MIME "text" MUST represent a line break. Use of CR and LF outside of line break sequences is also forbidden."
    This is for Content-Type: text/plain, but I don't know what Content-Type we are using in the errors.
  • as a separate note, I also notice an extra "\n" on the check... as in c.Check(body, check.Equals, notFoundMessage+"\n") any reason why is there?

services/keep-web/s3_test.go:

I noticed the checks are comparing the body only. It will be good to add a comparison of the statusCode too.. since clients could be only checking that.

Actions #21

Updated by Peter Amstutz over 3 years ago

  • Status changed from New to In Progress
Actions #22

Updated by Peter Amstutz over 3 years ago

Nico César wrote:

review at aa1c0f3049f7b78e7590dde868b915bef9a7ebbe

services/keep-web/handler.go:

  • Maybe I'm too picky here, but any reason to have "\n" in the errors? Would this cause issues on non-Unix systems that use "\r\n" like Windows? also reading RFC 2046 https://www.ietf.org/rfc/rfc2046.txt section 4.1.1 says: "The canonical form of any MIME "text" subtype MUST always represent a line break as a CRLF sequence. Similarly, any occurrence of CRLF in MIME "text" MUST represent a line break. Use of CR and LF outside of line break sequences is also forbidden."
    This is for Content-Type: text/plain, but I don't know what Content-Type we are using in the errors.

Sure. I don't think this is a problem in practice but I fixed it anyway.

  • as a separate note, I also notice an extra "\n" on the check... as in c.Check(body, check.Equals, notFoundMessage+"\n") any reason why is there?

The responses use http.Error() which calls Fprintln() which adds a newline to the end of the message:

https://golang.org/src/net/http/server.go?s=63178:63230#L2041

So the string comparison has to add the newline added by Fprintln().

services/keep-web/s3_test.go:

I noticed the checks are comparing the body only. It will be good to add a comparison of the statusCode too.. since clients could be only checking that.

I added checks for the codes in addition to the error messages.

16774-keep-web-errors @ 7d94b1ed55350c01689ad048aee961b261263dd9

developer-run-tests: #2196

Actions #23

Updated by Nico César over 3 years ago

review @ 7d94b1ed55350c01689ad048aee961b261263dd9

I think that \n or \r\n should be avoided from the message at all leaving as constants since 401 status code will come with them message:

const notFoundMessage = "The requested path was not found, or you do not have permission to access it." 
const unauthorizedMessage = "A valid Arvados token must be provided to access this resource." 

But either way is good to merge.

Actions #24

Updated by Peter Amstutz over 3 years ago

  • Status changed from In Progress to Resolved
Actions #25

Updated by Peter Amstutz over 3 years ago

  • Release set to 37
Actions

Also available in: Atom PDF