From 631f30b69e0f14194ae2321cc42e2977220edf34 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Thu, 29 Sep 2022 00:08:15 +0200 Subject: [PATCH 1/6] docs/api-description.json: Emphasize Cookie auth Increases API version to 4.0.1. `GET /login` and passing credentials in query string is now officially deprecated. --- NEWS.md | 3 +++ docs/api-description.json | 4 ++-- index.php | 2 +- src/constants.php | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/NEWS.md b/NEWS.md index 93d573ba71..c5617381de 100644 --- a/NEWS.md +++ b/NEWS.md @@ -49,6 +49,9 @@ - API is now versioned separately from selfoss and follows [semantic versioning](https://semver.org/) ([#1137](https://github.com/fossar/selfoss/pull/1137)) - *API 2.21.0*: `/mark` now accepts list of item IDs encoded as JSON. Requests using `application/x-www-form-urlencoded` are deprecated. ([#1182](https://github.com/fossar/selfoss/pull/1182)) - Dates returned as part of items now strictly follow ISO8601 format. ([#1246](https://github.com/fossar/selfoss/pull/1246)) +- The following are deprecated and will be removed in next selfoss version: + - Passing credentials in query string, use cookies instead. ([#1360](https://github.com/fossar/selfoss/pull/1360)) + - `GET /login` endpoint, use `POST /login`. ([#1360](https://github.com/fossar/selfoss/pull/1360)) ### Customization changes - `selfoss.shares.register` was removed. Instead you should set `selfoss.customSharers` to an object of *sharer* objects. The `action` callback is now expected to open a window on its own, instead of returning a URL. A label and a HTML code of an icon (you can use a `` tag, inline ``, emoji, etc.) are now expected. diff --git a/docs/api-description.json b/docs/api-description.json index 808c7734a3..1d45c459fa 100644 --- a/docs/api-description.json +++ b/docs/api-description.json @@ -3,13 +3,13 @@ "servers": [], "info": { "description": "You can access selfoss by using the same backend as selfoss user interface: The RESTful HTTP JSON API. There are a few urls where you can get information from selfoss and some for updating data. Assume you want all tags for rendering this in your own app. You have to make an HTTP GET call on the url /tags:\n\n```\nGET http://yourselfossurl.com/tags\n```\nThe result is following JSON formatted response (in this example two tags “blog” and “deviantart” are available:\n\n```\n[{\"tag\":\"blog\",\"color\":\"#251f10\",\"unread\":\"1\"},\n{\"tag\":\"deviantart\",\"color\":\"#e78e5c\",\"unread\":\"0\"}]\n```\n\nFollowing docs shows you which calls are possible and which response you can expect.", - "version": "4.0.0", + "version": "4.0.1", "title": "selfoss" }, "tags": [ { "name": "Authentication", - "description": "When you are using a login protected page, you have to add the parameter `username` and `password` in every request (as GET or POST parameter). The logical consequence is that you have to use https.\n\nFo0r an initial login functionality in your app you can validate a username password combination using `GET /login`.\n\nAlternately, you can use session cookie on subsequent requests." + "description": "To access endpoints that require authentication, you need to pass a session [cookie](https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies) with the request. To obtain it, sign in using `POST /login` API call – the HTTP response will contain a `Set-Cookie` header for `PHPSESSID` cookie, whose value you should store and pass it to the server on all subsequent API requests, using the `Cookie` header. Those requests can also generate a new session id and pass it to you again using `Set-Cookie` header in the response, in which case you should replace the stored value. The HTTP library you use will probably have a cookie jar support to handle this for you." } ], "paths": { diff --git a/index.php b/index.php index 2a763a6b9a..abffa093a6 100644 --- a/index.php +++ b/index.php @@ -20,7 +20,7 @@ $dice->create(controllers\Authentication::class)->password(); }); $router->get('/login', function() use ($dice) { - // json + // json, deprecated $dice->create(controllers\Authentication::class)->login(); }); $router->post('/login', function() use ($dice) { diff --git a/src/constants.php b/src/constants.php index 0e069a2294..b70fd56017 100644 --- a/src/constants.php +++ b/src/constants.php @@ -7,4 +7,4 @@ // independent of selfoss version // needs to be bumped each time public API is changed (follows semver) // keep in sync with docs/api-description.json -const SELFOSS_API_VERSION = '4.0.0'; +const SELFOSS_API_VERSION = '4.0.1'; From a8b820ecc988b98d3a3bcf2dd357d6e466ca09a8 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Thu, 29 Sep 2022 01:28:11 +0200 Subject: [PATCH 2/6] api: replace `GET /logout` with `DELETE /api/session/current` and deprecate the former. Increases API version to 4.1.0. Using `GET` method could, in theory, allow a limited DOS attack. While selfoss should absolutize all relative image `src` attributes when fetching a source, there may be bugs. Or less likely, a malicious feed could guess the domain and use the absolute URL of `/logout`. Or if user runs another app in the same context, it could be similarly hijacked. All those should probably be resolved by other means (e.g. CORS headers) but I doubt anyone will target selfoss users in this way just to annoy them. `DELETE` method is as close to REST as we can get with session state. See also the discussion on https://stackoverflow.com/questions/3521290/logout-get-or-post Also fix the API docs which incorrectly claimed the `/logout` works over `POST`. --- NEWS.md | 1 + assets/js/helpers/ajax.js | 3 +++ assets/js/requests/common.js | 2 +- docs/api-description.json | 27 +++++++++++++++++++++++++-- index.php | 4 ++++ src/constants.php | 2 +- 6 files changed, 35 insertions(+), 4 deletions(-) diff --git a/NEWS.md b/NEWS.md index c5617381de..44f33952a3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -52,6 +52,7 @@ - The following are deprecated and will be removed in next selfoss version: - Passing credentials in query string, use cookies instead. ([#1360](https://github.com/fossar/selfoss/pull/1360)) - `GET /login` endpoint, use `POST /login`. ([#1360](https://github.com/fossar/selfoss/pull/1360)) + - `GET /logout` was deprecated in favour of newly introduced (*API 4.1.0*) `DELETE /api/session/current`. ([#1360](https://github.com/fossar/selfoss/pull/1360)) ### Customization changes - `selfoss.shares.register` was removed. Instead you should set `selfoss.customSharers` to an object of *sharer* objects. The `action` callback is now expected to open a window on its own, instead of returning a URL. A label and a HTML code of an icon (you can use a `` tag, inline ``, emoji, etc.) are now expected. diff --git a/assets/js/helpers/ajax.js b/assets/js/helpers/ajax.js index ea1de3c033..a6e74d4818 100644 --- a/assets/js/helpers/ajax.js +++ b/assets/js/helpers/ajax.js @@ -176,6 +176,9 @@ export const get = liftToPromiseField(option('method', 'GET'))(fetch); export const post = liftToPromiseField(option('method', 'POST'))(fetch); +export const delete_ = liftToPromiseField(option('method', 'DELETE'))(fetch); + + /** * Using URLSearchParams directly handles dictionaries inconveniently. * For example, it joins arrays with commas or includes undefined keys. diff --git a/assets/js/requests/common.js b/assets/js/requests/common.js index 9fe4821bec..5ec0d9aace 100644 --- a/assets/js/requests/common.js +++ b/assets/js/requests/common.js @@ -23,5 +23,5 @@ export function login(credentials) { * Terminates the active user session. */ export function logout() { - return ajax.get('logout').promise; + return ajax.delete_('api/session/current').promise; } diff --git a/docs/api-description.json b/docs/api-description.json index 1d45c459fa..7d5435cafd 100644 --- a/docs/api-description.json +++ b/docs/api-description.json @@ -3,7 +3,7 @@ "servers": [], "info": { "description": "You can access selfoss by using the same backend as selfoss user interface: The RESTful HTTP JSON API. There are a few urls where you can get information from selfoss and some for updating data. Assume you want all tags for rendering this in your own app. You have to make an HTTP GET call on the url /tags:\n\n```\nGET http://yourselfossurl.com/tags\n```\nThe result is following JSON formatted response (in this example two tags “blog” and “deviantart” are available:\n\n```\n[{\"tag\":\"blog\",\"color\":\"#251f10\",\"unread\":\"1\"},\n{\"tag\":\"deviantart\",\"color\":\"#e78e5c\",\"unread\":\"0\"}]\n```\n\nFollowing docs shows you which calls are possible and which response you can expect.", - "version": "4.0.1", + "version": "4.1.0", "title": "selfoss" }, "tags": [ @@ -99,7 +99,30 @@ } }, "/logout": { - "post": { + "get": { + "deprecated": true, + "tags": [ + "Authentication" + ], + "summary": "Deauthenticate the user", + "description": "Destroys the session on the server", + "operationId": "logoutLegacy", + "responses": { + "200": { + "description": "always true", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Response" + } + } + } + } + } + } + }, + "/api/session/current": { + "delete": { "tags": [ "Authentication" ], diff --git a/index.php b/index.php index abffa093a6..57ef59a8cc 100644 --- a/index.php +++ b/index.php @@ -28,6 +28,10 @@ $dice->create(controllers\Authentication::class)->login(); }); $router->get('/logout', function() use ($dice) { + // json, deprecated + $dice->create(controllers\Authentication::class)->logout(); +}); +$router->delete('/api/session/current', function() use ($dice) { // json $dice->create(controllers\Authentication::class)->logout(); }); diff --git a/src/constants.php b/src/constants.php index b70fd56017..7ea42fb2dc 100644 --- a/src/constants.php +++ b/src/constants.php @@ -7,4 +7,4 @@ // independent of selfoss version // needs to be bumped each time public API is changed (follows semver) // keep in sync with docs/api-description.json -const SELFOSS_API_VERSION = '4.0.1'; +const SELFOSS_API_VERSION = '4.1.0'; From 5152d5d61328b61a1974237bc8ed1dd9f48fc042 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Thu, 29 Sep 2022 01:37:22 +0200 Subject: [PATCH 3/6] api: Deprecate `POST /source/delete/:id` It was actually introduced after `DELETE /source/:id`: https://github.com/fossar/selfoss/commit/205027297ae75b08bc24e074c561092ebea9f003 --- NEWS.md | 1 + assets/js/requests/sources.js | 2 +- index.php | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index 44f33952a3..6f3bc0364a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -53,6 +53,7 @@ - Passing credentials in query string, use cookies instead. ([#1360](https://github.com/fossar/selfoss/pull/1360)) - `GET /login` endpoint, use `POST /login`. ([#1360](https://github.com/fossar/selfoss/pull/1360)) - `GET /logout` was deprecated in favour of newly introduced (*API 4.1.0*) `DELETE /api/session/current`. ([#1360](https://github.com/fossar/selfoss/pull/1360)) + - `POST /source/delete/:id` in favour of `DELETE /source/:id`. ([#1360](https://github.com/fossar/selfoss/pull/1360)) ### Customization changes - `selfoss.shares.register` was removed. Instead you should set `selfoss.customSharers` to an object of *sharer* objects. The `action` callback is now expected to open a window on its own, instead of returning a URL. A label and a HTML code of an icon (you can use a `` tag, inline ``, emoji, etc.) are now expected. diff --git a/assets/js/requests/sources.js b/assets/js/requests/sources.js index 2dab715ddf..c5650360ce 100644 --- a/assets/js/requests/sources.js +++ b/assets/js/requests/sources.js @@ -34,7 +34,7 @@ export function refreshAll() { * Removes source with given ID. */ export function remove(id) { - return ajax.post(`source/delete/${id}`).promise; + return ajax.delete_(`source/${id}`).promise; } /** diff --git a/index.php b/index.php index 57ef59a8cc..d8e6330e7d 100644 --- a/index.php +++ b/index.php @@ -125,7 +125,7 @@ $dice->create(controllers\Sources::class)->remove($id); }); $router->post('/source/delete/([0-9]+)', function($id) use ($dice) { - // json + // json, deprecated $dice->create(controllers\Sources::class)->remove($id); }); $router->post('/source/([0-9]+)/update', function($id) use ($dice) { From d1b04af1ba13be3f8cf66959a60a1b02e9b60874 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Fri, 30 Sep 2022 16:23:48 +0200 Subject: [PATCH 4/6] api/update: Return 403 Forbidden on unallowed Make the error code explicit. This is undocumented API but we are still raising API version since apps might have been relying on `200 OK` status code for unallowed access. --- docs/api-description.json | 2 +- src/constants.php | 2 +- src/controllers/Sources/Update.php | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/api-description.json b/docs/api-description.json index 7d5435cafd..bed1f9d7d2 100644 --- a/docs/api-description.json +++ b/docs/api-description.json @@ -3,7 +3,7 @@ "servers": [], "info": { "description": "You can access selfoss by using the same backend as selfoss user interface: The RESTful HTTP JSON API. There are a few urls where you can get information from selfoss and some for updating data. Assume you want all tags for rendering this in your own app. You have to make an HTTP GET call on the url /tags:\n\n```\nGET http://yourselfossurl.com/tags\n```\nThe result is following JSON formatted response (in this example two tags “blog” and “deviantart” are available:\n\n```\n[{\"tag\":\"blog\",\"color\":\"#251f10\",\"unread\":\"1\"},\n{\"tag\":\"deviantart\",\"color\":\"#e78e5c\",\"unread\":\"0\"}]\n```\n\nFollowing docs shows you which calls are possible and which response you can expect.", - "version": "4.1.0", + "version": "5.0.0", "title": "selfoss" }, "tags": [ diff --git a/src/constants.php b/src/constants.php index 7ea42fb2dc..30bb26cb23 100644 --- a/src/constants.php +++ b/src/constants.php @@ -7,4 +7,4 @@ // independent of selfoss version // needs to be bumped each time public API is changed (follows semver) // keep in sync with docs/api-description.json -const SELFOSS_API_VERSION = '4.1.0'; +const SELFOSS_API_VERSION = '5.0.0'; diff --git a/src/controllers/Sources/Update.php b/src/controllers/Sources/Update.php index 6ff65c7ee8..0c2f9a2cca 100644 --- a/src/controllers/Sources/Update.php +++ b/src/controllers/Sources/Update.php @@ -29,6 +29,7 @@ public function __construct(Authentication $authentication, ContentLoader $conte public function updateAll() { // only allow access for localhost and loggedin users if (!$this->authentication->allowedToUpdate()) { + header('HTTP/1.0 403 Forbidden'); exit('unallowed access'); } @@ -49,6 +50,7 @@ public function updateAll() { public function update($id) { // only allow access for localhost and authenticated users if (!$this->authentication->allowedToUpdate()) { + header('HTTP/1.0 403 Forbidden'); exit('unallowed access'); } From 7a0aff6e7af2e8f87ae0ee7b5470ba25d1964095 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Tue, 4 Oct 2022 14:45:43 +0200 Subject: [PATCH 5/6] client: Fix MessageFormat string placeholders --- assets/js/helpers/i18n.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/assets/js/helpers/i18n.js b/assets/js/helpers/i18n.js index b48e9bb686..4756a1bc00 100644 --- a/assets/js/helpers/i18n.js +++ b/assets/js/helpers/i18n.js @@ -42,7 +42,9 @@ export function i18nFormat(translated, params) { case ',': if (placeholder) { if (state == 'index') { - placeholder.index = parseInt(buffer.trim()); + const index = buffer.trim(); + const intIndex = parseInt(index); + placeholder.index = Number.isNaN(intIndex) ? index : intIndex; placeholder.value = params[placeholder.index]; buffer = ''; } else if (state == 'type') { From b61c6a2c65b98dd042ec3fdcdf11b51cdb97e635 Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Tue, 4 Oct 2022 14:45:05 +0200 Subject: [PATCH 6/6] client: Distinguish login failure from server error The 403 or 500 HTTP errors have nothing to do with selfoss credentials so we should show a different error. While at it, move the error detection to the request function and remove redundant redirect. --- assets/js/errors.js | 7 +++++++ assets/js/requests/common.js | 11 ++++++++++- assets/js/selfoss-base.js | 18 ++++++------------ assets/js/templates/LoginForm.jsx | 14 ++++++++++++-- assets/locale/cs.json | 1 + assets/locale/en.json | 1 + 6 files changed, 37 insertions(+), 15 deletions(-) diff --git a/assets/js/errors.js b/assets/js/errors.js index d7bfaa7d6b..6a59956916 100644 --- a/assets/js/errors.js +++ b/assets/js/errors.js @@ -18,3 +18,10 @@ export class HttpError extends Error { this.name = 'HttpError'; } } + +export class LoginError extends Error { + constructor(message) { + super(message); + this.name = 'LoginError'; + } +} diff --git a/assets/js/requests/common.js b/assets/js/requests/common.js index 5ec0d9aace..4839af160d 100644 --- a/assets/js/requests/common.js +++ b/assets/js/requests/common.js @@ -1,3 +1,4 @@ +import { LoginError } from '../errors'; import * as ajax from '../helpers/ajax'; /** @@ -16,7 +17,15 @@ export function getInstanceInfo() { export function login(credentials) { return ajax.post('login', { body: new URLSearchParams(credentials) - }).promise.then(response => response.json()); + }).promise.then( + response => response.json() + ).then((data) => { + if (data.success) { + return Promise.resolve(); + } else { + return Promise.reject(new LoginError(data.error)); + } + }); } /** diff --git a/assets/js/selfoss-base.js b/assets/js/selfoss-base.js index 9affe77a3a..e21d1269ce 100644 --- a/assets/js/selfoss-base.js +++ b/assets/js/selfoss-base.js @@ -199,19 +199,13 @@ var selfoss = { username, password }; - return login(credentials).then((data) => { - if (data.success) { - selfoss.setSession(); - selfoss.history.push('/'); - // init offline if supported and not inited yet + return login(credentials).then(() => { + selfoss.setSession(); + // init offline if supported and not inited yet + selfoss.dbOffline.init(); + if ((!selfoss.db.storage || selfoss.db.broken) && selfoss.db.enableOffline.value) { + // Initialize database in offline mode when it has not been initialized yet or it got broken. selfoss.dbOffline.init(); - if ((!selfoss.db.storage || selfoss.db.broken) && selfoss.db.enableOffline.value) { - // Initialize database in offline mode when it has not been initialized yet or it got broken. - selfoss.dbOffline.init(); - } - return Promise.resolve(); - } else { - return Promise.reject(new Error(data.error)); } }); }, diff --git a/assets/js/templates/LoginForm.jsx b/assets/js/templates/LoginForm.jsx index 576c0880f0..c182bc40fb 100644 --- a/assets/js/templates/LoginForm.jsx +++ b/assets/js/templates/LoginForm.jsx @@ -3,6 +3,7 @@ import PropTypes from 'prop-types'; import classNames from 'classnames'; import { SpinnerBig } from './Spinner'; import { useHistory, useLocation } from 'react-router-dom'; +import { HttpError, LoginError } from '../errors'; import { ConfigurationContext } from '../helpers/configuration'; import { LocalizationContext } from '../helpers/i18n'; @@ -20,9 +21,18 @@ function handleLogIn({ selfoss.login({ username, password, offlineEnabled }).then(() => { history.push('/'); - }).catch(() => { + }).catch((err) => { + const message = + err instanceof LoginError + ? selfoss.app._('login_invalid_credentials') + : selfoss.app._('login_error_generic', { + errorMessage: + err instanceof HttpError + ? `HTTP ${err.response.status} ${err.message}` + : err.message, + }); history.replace('/sign/in', { - error: selfoss.app._('login_invalid_credentials') + error: message, }); }).finally(() => { setLoading(false); diff --git a/assets/locale/cs.json b/assets/locale/cs.json index 5d0cc8df43..b0f02a84fe 100644 --- a/assets/locale/cs.json +++ b/assets/locale/cs.json @@ -65,6 +65,7 @@ "lang_login_username": "Jméno", "lang_login_password": "Heslo", "lang_login_invalid_credentials": "Špatné jméno nebo heslo", + "lang_login_error_generic": "Pokus o přihlášení selhal: {errorMessage}", "lang_no_title": "Bez titulku", "lang_error": "Nastala chyba", "lang_error_unauthorized": "Pro pokračování se prosím {link_begin}přihlaste{link_end}.", diff --git a/assets/locale/en.json b/assets/locale/en.json index 1fc2a92bc9..7f2880b89b 100644 --- a/assets/locale/en.json +++ b/assets/locale/en.json @@ -77,6 +77,7 @@ "lang_login_username": "Username", "lang_login_password": "Password", "lang_login_invalid_credentials": "Wrong username/password", + "lang_login_error_generic": "Could not log in: {errorMessage}", "lang_login_offline": "Offline storage", "lang_no_title": "No title", "lang_experimental": "Experimental",