diff options
Diffstat (limited to 'server')
| -rw-r--r-- | server/internal/api/api.go | 6 | ||||
| -rw-r--r-- | server/internal/api/api_test.go | 44 | ||||
| -rw-r--r-- | server/internal/api/transcript_test.go | 9 |
3 files changed, 54 insertions, 5 deletions
diff --git a/server/internal/api/api.go b/server/internal/api/api.go index d716bb4..2d2e279 100644 --- a/server/internal/api/api.go +++ b/server/internal/api/api.go @@ -278,8 +278,10 @@ func (s *Server) transcriptNoteFor(e *store.Episode) *transcriptNote { Note: e.TranscriptNote, } if e.HasTranscript() { - note.URL = "/api/transcript?episode=" + url.QueryEscape(e.ID) + - "&token=" + url.QueryEscape(s.opts.Token) + // No token. The phone sends Authorization: Bearer on this request anyway and tokenOK + // takes either, so putting it in the URL bought nothing and cost a copy of the token in + // every line of Apache's access log -- which keeps fourteen rotations. + note.URL = "/api/transcript?episode=" + url.QueryEscape(e.ID) if held, err := (transcript.Store{Dir: s.opts.TranscriptDir}).Get(e.ID); err == nil { note.Ads = len(held.Ads) for _, ad := range held.Ads { diff --git a/server/internal/api/api_test.go b/server/internal/api/api_test.go index 8a29b33..b50454b 100644 --- a/server/internal/api/api_test.go +++ b/server/internal/api/api_test.go @@ -441,3 +441,47 @@ func TestAskingForAnEpisodeMarksItWanted(t *testing.T) { t.Error("asking for a mirrored episode left it unwanted, so a poll may still drop it") } } + +/* +The transcript URL carries no token. + +Apache logs the request line, so every /api/transcript fetch wrote the server's token into +listen.terst.org-access.log in plaintext, across fourteen rotations. The token was redundant +there in the first place: the phone sends Authorization: Bearer on this request anyway, through +OkHttpSkipTransport, and tokenOK accepts either. + +The audio URL still carries one. That one is not redundant -- the downloader fetches it with no +credentials of its own -- and removing it needs the app to send the header first, so it goes in +a later change. +*/ +func TestTheTranscriptURLDoesNotCarryTheToken(t *testing.T) { + s, st, _, audioDir := newServer(t) + seedMirrored(t, st, audioDir, "audio-bytes") + e := st.Match("https://f", "g1", "", "") + e.TranscriptSource, e.TranscriptCues = "asr", 12 + + rec := get(t, s, "/api/episode?feed=https://f&guid=g1") + + var got episodeResponse + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if got.Transcript == nil || got.Transcript.URL == "" { + t.Fatalf("no transcript url in %s", rec.Body.String()) + } + if strings.Contains(got.Transcript.URL, "token") { + t.Errorf("the token is in the transcript url, and so in every access log line: %s", got.Transcript.URL) + } +} + +// And the request still works, because the header is what authorises it. +func TestTheTranscriptEndpointTakesTheHeader(t *testing.T) { + s, st, _, audioDir := newServer(t) + seedMirrored(t, st, audioDir, "audio-bytes") + + rec := get(t, s, "/api/transcript?episode="+episodeID(st)) + + if rec.Code == http.StatusUnauthorized || rec.Code == http.StatusForbidden { + t.Errorf("a bearer header must authorise this; got %d", rec.Code) + } +} diff --git a/server/internal/api/transcript_test.go b/server/internal/api/transcript_test.go index e6da3ff..dff4d54 100644 --- a/server/internal/api/transcript_test.go +++ b/server/internal/api/transcript_test.go @@ -106,9 +106,12 @@ func TestTheResponseSaysWhereTheWordsAre(t *testing.T) { if got.Transcript == nil || !strings.Contains(got.Transcript.URL, e.ID) { t.Fatalf("got %+v, want a URL naming the episode", got.Transcript) } - // Usable as given, like the audio URL: the phone appends nothing. - if !strings.Contains(got.Transcript.URL, "token=") { - t.Errorf("url = %q, want it to carry the token", got.Transcript.URL) + // Not usable as given, deliberately. The phone authorises this with a bearer header, which + // it was sending anyway, and a token in the URL was a copy of the secret in every line of + // Apache's access log. The audio URL still carries one because the downloader has no + // credentials of its own; see TestTheTranscriptURLDoesNotCarryTheToken. + if strings.Contains(got.Transcript.URL, "token=") { + t.Errorf("url = %q, want no token in it", got.Transcript.URL) } } |
