-
Notifications
You must be signed in to change notification settings - Fork 753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Google Reader API: Add feed icon URL endpoint #3195
base: main
Are you sure you want to change the base?
Conversation
What's the point of this? |
@jvoisin I went with this direction given a comment from the initial "Share article" PR #475 (comment)
So in the same way, this avoids exposing the internal Icon or Feed IDs for these endpoints. |
d3b1264
to
b0304de
Compare
|
||
icon, err := h.store.IconByHash(iconHash) | ||
|
||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API call should probably return a 404 if the icon hash is not found in the database instead of 500 with the body {"error_message":"store: unable to fetch icon: sql: no rows in result set"}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. This should now return the standard 404 message
// GET http://localhost:8080/reader/api/0/icon/foobar
{
"error_message": "resource not found"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating IconByHash()
to take sql.ErrNoRows
into consideration is more appropriate because we need to make the distinction between a database error and no result found.
Look at store.IconByID()
and api.getIconByIconID()
if you need an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha, I re-added the 500 handling and added in 404 handling that will also catch sql.ErrNoRows
.
internal/googlereader/handler.go
Outdated
builder := response.New(w, r) | ||
builder.WithHeader("Content-Type", icon.MimeType) | ||
builder.WithBody(icon.Content) | ||
builder.Write() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to use HTTP caching similar to the web ui in feed_icon.go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I copied over that code but happy to work out an abstraction if desired.
Testing in my browser, the second call correctly returns 304 Not Modified
.
8689193
to
e52949d
Compare
Adds an endpoint to the Google Reader integration to serve feed icon URLs.
e52949d
to
d8a1df1
Compare
Then you might want to use something like an HMAC instead, as an attacker could simply download a bunch of favicon from popular feeds, and check if they exist on the navidrome instance. |
HMAC could work. I'll take another pass at this. |
HMAC requires a secret key. How would that be implemented? |
There's a few options, really open to any: Option 1: Integration level Add a secret to the ALTER TABLE integrations ADD COLUMN googlereader_salt text ...; On result, check The upside is that this would only add a single row to the database, and only populate it if the Google Reader integration is enabled. The downside is that it's relatively resource intensive: several SQL queries, and re-computation of HMACs on each request. Option 2: Icon level With this option, the secret isn't added as an HMAC, it's generated alongside each icon either as a random string or something like a UUIDv4. The generated URL will use the unique, random alphanumeric value in the same way that the "share article": ALTER TABLE icons ADD COLUMN share_code text ...; The subscription list will use The upside with this approach is that it's much lighter. No extra hashing involved. The downside is that it shifts the complexity from computation to database storage. All things equal, option 2 would be my approach to avoid hashing and heavy SQL queries. |
Adds an endpoint to the Google Reader integration to serve feed icon URLs.
The
/icon/{iconHash}
route is an unauthorized endpoint. This is consistent with other feed readers like FreshRSS (icon source code).To obfuscate the Icon ID, the icon hash is used instead. The SHA-256 hash should generate a hexadecimal value which is url safe.
The subscription output looks something like this:
Have you followed these guidelines?