Update CSRF token handling #179
Conversation
| fetchTokenRequest, err := http.NewRequest(http.MethodGet, url, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| return csrf_parameters.CsrfParams{}, err | ||
| } | ||
| fetchTokenRequest.Header.Set(XCsrfToken, CsrfTokenHeaderFetchValue) |
There was a problem hiding this comment.
It's an old code, but I do not think this "Fetch" value is required on the backend
clients/csrf/csrf.go
Outdated
|
|
||
| const CookieHeader = "Cookie" | ||
|
|
||
| type Csrf struct { |
There was a problem hiding this comment.
csrf is not very descriptive, maybe rename to
CsrfTokenHelper
CsrfTokenUpdater
| return nil | ||
| } | ||
| c.transport.Csrf.Header, c.transport.Csrf.Token = csrfToken.CsrfTokenHeader, csrfToken.CsrfTokenValue | ||
| if c.csrf.IsExpired(CsrfSessionTimeout) { |
There was a problem hiding this comment.
I think this is brittle.
Yes, the session timeout is 2 minutes and the token is per session, but there is some time lost during routing, server processing of the returned token, etc. There might be a case when the token has less than 2 minutes of expiration left.
I'd say 1:30 minutes max. We've seen some fetch token requests that took 10 seconds and nobody knows where exactly was the delay and when the token/session was created
There was a problem hiding this comment.
The less the safer
clients/csrf/transport.go
Outdated
| csrfTokenManager := NewDefaultCsrfTokenManager(t) | ||
| err := csrfTokenManager.updateToken(reqCopy) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| log.Tracef("Sending a request with CSRF '%s : %s'\n", reqCopy.Header.Get("X-Csrf-Header"), reqCopy.Header.Get("X-Csrf-Token")) | ||
| log.Tracef("Sending a request with CSRF %s\n", reqCopy.Header.Get("X-Csrf-Token")) | ||
| log.Tracef("Cookies used are: %s\n", prettyPrintCookies(reqCopy.Cookies())) | ||
| res, err := t.OriginalTransport.RoundTrip(reqCopy) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| tokenWasRefreshed, err := csrfTokenManager.refreshTokenIfNeeded(res) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if tokenWasRefreshed { | ||
| log.Tracef("Response code '%d' from bad token. Must Retry.\n", res.StatusCode) | ||
| return nil, &ForbiddenError{} | ||
| } | ||
| return res, err | ||
| return t.Delegate.RoundTrip(reqCopy) |
There was a problem hiding this comment.
I think this code handled the case when DS instance is restarted and the token and session are lost.
Now this case is not handled.
The DS will return a header:
"X-CSRF-TOKEN: Required", when Invalid/Missing Csrf exception occurs. We can check this header and re-init the token and re-execute the request.
| h := make(textproto.MIMEHeader) | ||
| h.Set("Content-Disposition", fmt.Sprintf(`form-data; name="file"; filename="%s"`, escapeQuotes(fileName))) | ||
| h.Set("Content-Type", "application/octet-stream") | ||
| h.Set("Content-Length", strconv.FormatInt(fileSize, 10)) |
There was a problem hiding this comment.
What will be difference when Content-Legth is set?
There was a problem hiding this comment.
If Tomcat was smart, it would read this length and set it in the Part object as its size, instead of persisting the entire file to disk.
| h := make(textproto.MIMEHeader) | ||
| h.Set("Content-Disposition", fmt.Sprintf(`form-data; name="file"; filename="%s"`, escapeQuotes(file.Name()))) | ||
| h.Set("Content-Type", "application/octet-stream") | ||
| h.Set("Content-Length", strconv.FormatInt(fileSize, 10)) | ||
| fileWriter, err := form.CreatePart(h) | ||
| if err != nil { | ||
| return fmt.Errorf("could not write to HTTP request: %v", err) | ||
| return fmt.Errorf("could not create multipart file part: %v", err) |
There was a problem hiding this comment.
This can be split into a new method and reused.
Change the csrf token handling flow.
Keep a timestamp of the last token update time and fetch a new one if the csrf session has expired before sending a request.