What is the problem?
Developers need to profile pages while browsing as anonymous users or
while impersonating other users. Currently, MiniProfiler authorization
is tied to the session, so it's lost when the session changes.
What is the solution?
Add a `/dev-mode` endpoint that sets an encrypted cookie to persist
MiniProfiler authorization for 1 hour, independent of the session:
- New `DevModeController` with CSRF-protected POST form
- Only accessible to users in the Developer group
- Cookie validated on each request by checking timestamp, user
existence, and developer status
Meta:
https://meta.discourse.org/t/set-locale-from-param-and-content-localisation-issue/387547
We want to make sure that the `SiteSetting.set_locale_from_param`
conforms to its description: `"Allows setting an anonymous user's locale
via the 'tl' URL param, e.g. ?tl=es"`. Currently, the bug is that this
is applied to logged in users as well.
We also do not want to modify a logged in user's locale as it should
remain as per their user profile.
By default, we want all responses to have the `Cache-Control` header set
to `no-cache, no-store`. Individual controller actions can override the
header when need be.
Since the session is backed by a cookie, storing too much data will lead
to a cookie overflow error.
A return path can be quite large sometimes, so intead of storing it in
the session, this patch stores it in our server session.
This patch will be followed by
https://github.com/discourse/discourse/pull/34747.
`SecureSession` doesn’t make a lot of sense anymore and can be confusing
as the current cookie store used for the session is actually secure
since it’s encrypted.
Renaming it to `ServerSession` better conveys what it does: providing a
session but on the server side only.
This patch also makes some improvements, like injecting that server
session into Rack-like request objects, allowing the server session to
be available virtually everywhere.
Since the introduction of dedicated login and signup pages (as opposed
to modals), we've been seeing reports of issues where visitors aren't
redirected back to the "page" they were at when they initiated the
_authentication_ process.
Since we have a bazillion of ways a user might authenticate
(credentials, social logins, SSO, passkeys, discourse connect, etc...),
it's really hard to know what a change will impact.
The goal of this PR is to "simplify" the way we handle this "redirection
back to origin" by leveraging the use of a single `destination_url`
cookie set on the client-side.
The changes remove scattered cookie-setting code and consolidate the redirection logic to ensure users are properly redirected back to their original page after authentication.
- Centralized destination URL cookie management in routes and authentication flows
- Removed manual cookie setting from various components in favor of automatic handling
- Updated test scenarios to properly test the new redirection behavior
fixes errors like:
```
Failed to handle exception in exception app middleware : NoMethodError : undefined method `ends_with?' for an instance of Integer
```
The reasons for these changes is https://meta.discourse.org/t/-/89605
broke and admins were not able to log back in if they had previously
enabled the "read only" mode.
Thus ensued a deep dive into how all the "read only" modes worked, which
was made difficult due to the lack of tests.
The "cornerstone" of this PR is the `read_only_mixin.rb` file which was
improved to be able to differentiate between the "readonly" mode and the
"staff writes only" mode.
I then made use of the `allow_in_readonly_mode` and
`allow_in_staff_writes_only_mode` method to **explicitely** list all the
actions that should work in those modes.
I also added the "readonly" mixin to the `WebhooksController` since it
doesn't inherit from the `ApplicationController`.
I improved the security of the `/u/admin-login` endpoint by always
sending the same message no matter if we found or not an admin account
with the provided email address.
I added two system specs:
1. for ensuring that admins can log in via /u/admin-lgoin and then
clicking the link in the email they received while the site is in
readonly mode.
2. for ensuring the "staff writes only mode" is _actually_ tested by
ensuring a moderator can log in and create a topic while the site is in
that mode.
Plenty of specs were updated to ensure 100% converage of the various
"read only" modes.
Previously we would check the request for a matching CDN hostname before
applying the `Access-Control-Allow-Origin` header. That logic requires
the CDN to include its public-facing hostname in the `Host` header,
which is not always the case.
Since we are only running this `apply_cdn_headers` before_action on
publicly-accessible asset routes, we can simplify things so that the
`Access-Control-Allow-Origin: *` header is always included. That will
make CDN config requirements much more relaxed.
At the moment, this is primarily relevant to the HighlightJsController
routes, which are loaded using native JS `type=module`. But in the near
future, we plan to expand our use of `type=module` to more critical JS
assets like translations and themes.
Also drops the `Access-Control-Allow-Methods` header from these
responses. That isn't needed for `GET` and `HEAD` requests.
In some cases, Google crawlers don't output the meta description but
rather they output the first bit of text in the UI. Sometimes that is a
mix of table headings, topic titles and excerpts, which don't reflect
the site's mission. Adding the description to the homepage header might
help.
Internal ticket t/154372
This commit drops the `before_action :preload_json` callback in `ApplicationController` as it adds unnecessary complexity to `ApplicationController` as well as other controllers which has to skip this callback. The source of the complexity comes mainly from the following two conditionals in the `preload_json` method:
```
# We don't preload JSON on xhr or JSON request
return if request.xhr? || request.format.json?
# if we are posting in makes no sense to preload
return if request.method != "GET"
```
Basically, the conditionals solely exists for optimization purposes to ensure that we don't run the preloading code when the request is not a GET request and the response is not expected to be HTML. The key problem here is that the conditionals are trying to expect what the content type of the response will be and this has proven to be hard to get right. Instead, we can simplify this problem by running the preloading code in a more deterministic way which is to preload only when the `application` layout is being rendered and this is main change that this commit introduces.
This change adds a sidebar link for each plugin that fulfils the following criteria:
- Does not have an explicit admin route defined in the plugin.
- Has at least one site setting (not including enabled/disabled.)
That sidebar link leads to the automatically generated plugin show settings page.
Currently in services, we don’t make a distinction between input
parameters, options and dependencies.
This can lead to user input modifying the service behavior, whereas it
was not the developer intention.
This patch addresses the issue by changing how data is provided to
services:
- `params` is now used to hold all data coming from outside (typically
user input from a controller) and a contract will take its values from
`params`.
- `options` is a new key to provide options to a service. This typically
allows changing a service behavior at runtime. It is, of course,
totally optional.
- `dependencies` is actually anything else provided to the service (like
`guardian`) and available directly from the context object.
The `service_params` helper in controllers has been updated to reflect
those changes, so most of the existing services didn’t need specific
changes.
The options block has the same DSL as contracts, as it’s also based on
`ActiveModel`. There aren’t any validations, though. Here’s an example:
```ruby
options do
attribute :allow_changing_hidden, :boolean, default: false
end
```
And here’s an example of how to call a service with the new keys:
```ruby
MyService.call(params: { key1: value1, … }, options: { my_option: true }, guardian:, …)
```
When a user is missing required fields, they are required to fill those up before continuing to interact with the forum. This applies to admins as well.
We keep a whitelist of paths that can still be visited in this mode: FAQ, About, 2FA setup, and any admin route for admins.
We concluded that admins should still be able to enable safe mode even with missing required fields. Since plugins etc. can potentially mess with the ability to fill those up.
Constants should always be only assigned once. The logical OR assignment
of a constant is a relic of the past before we used zeitwerk for
autoloading and had bugs where a file could be loaded twice resulting in
constant redefinition warnings.
If a plugin's JS fails to load for some reason, most commonly
ad blockers, the entire admin interface would break. This is because
we are adding links to the admin routes for plugins that define
them in the sidebar.
We have a fix for this already in the plugin list which shows a warning
to the admin. This fix just prevents the broken link from rendering
in the sidebar if the route is not valid.
Previously in these 2 PRs, we introduced a new site setting `SiteSetting.enforce_second_factor_on_external_auth`.
https://github.com/discourse/discourse/pull/27547https://github.com/discourse/discourse/pull/27674
When disabled, it should enforce 2FA for local login with username and password and skip the requirement when authenticating with oauth2.
We stored information about the login method in a secure session but it is not reliable. Therefore, information about the login method is moved to the database.
Followup 3ff7ce78e7
Basing this setting on referrer was too brittle --
the referrer header can easily be ommitted or changed.
Instead, for the small amount of use cases that this
site setting serves, we can use a group-based setting
instead, changing it to `cross_origin_opener_unsafe_none_groups`
instead.
If the `enforce_second_factor_on_external_auth` setting
is disabled and a user logs in with an OAuth method,
they don't automatically get redirected to /preferences/second-factor
on login. However, they can get there manually, and once there
they cannot leave.
This commit fixes the issue and allows them to leave
and also does some refactors to indicate to the client
what login method is used as a followup to
0e1102b332
We want to allow admins to make new required fields apply to existing users. In order for this to work we need to have a way to make those users fill up the fields on their next page load. This is very similar to how adding a 2FA requirement post-fact works. Users will be redirected to a page where they can fill up the remaining required fields, and until they do that they won't be able to do anything else.
Followup to 0e1102b332
Minor followup, makes the condition check against the
boolean val, see the difference here:
```ruby
!SiteSetting.enforce_second_factor_on_external_auth && "true"
=> "true"
```
vs:
```ruby
!SiteSetting.enforce_second_factor_on_external_auth && "true" == "true"
=> true
```
This ensures that the theme id is resolved as early as possible in the
request cycle. This is necessary for the custom homepage to skip
preloading the wrong data.
In this PR we introduced `enforce_second_factor_on_external_auth` setting https://github.com/discourse/discourse/pull/27506
When it is set to false and the user is authenticated via OAuth, then we should not enforce the 2fa configuration.
Some tooling may rely on an unsafe-none cross origin opener policy to work. This change adds a hidden site setting that can be used to list referrers where we add this header instead of the default one configured in cross_origin_opener_policy_header.
This commit removes the `/admin-revamp` routes which were introduced as a part of an experiment to revamp the admin pages. We still want to improve the admin/staff experience, but we're going to do them within the existing `/admin` routes instead of introducing a completely new route.
Our initial efforts to improve the Discourse admin experience is this commit which introduces the foundation for a new subroute `/admin/config` which will house various new pages for configuring Discourse. The first new page (or "config area") will be `/admin/config/about` that will house all the settings and controls for configuring the `/about` page of Discourse.
Internal topic: t/128544
This commit moves the logic for crawler rate limits out of the application controller and into the request tracker middleware. The reason for this move is to apply rate limits to all crawler requests instead of just the requests that make it to the application controller. Some requests are served early from the middleware stack without reaching the Rails app for performance reasons (e.g. `AnonymousCache`) which results in crawlers getting 200 responses even though they've reached their limits and should be getting 429 responses.
Internal topic: t/128810.
- Run the CSP-nonce-related middlewares on the generated response
- Fix the readonly mode checking to avoid empty strings being passed (the `check_readonly_mode` before_action will not execute in the case of these re-dispatched exceptions)
- Move the BlockRequestsMiddleware cookie-setting to the middleware, so that it is included even for unusual HTML responses like these exceptions
Using around_action means `add_early_hint_header` is in the stack for every request, and gets included in the backtrace of any errors.
We can manage with an after_action instead, which avoids adding to the stack depth (and avoids people blaming me for unrelated application errors 😉)
This commit removes the 'experimental_preconnect_link_header' site setting, and the 'preload_link_header' site setting, and introduces two new global settings: early_hint_header_mode and early_hint_header_name.
We don't actually send 103 Early Hint responses from Discourse. However, upstream proxies can be configured to cache a response header from the app and use that to send an Early Hint response to future clients.
- `early_hint_header_mode` specifies the mode for the early hint header. Can be nil (disabled), "preconnect" (lists just CDN domains) or "preload" (lists all assets).
- `early_hint_header_name` specifies which header name to use for the early hint. Defaults to "Link", but can be changed to support different proxy mechanisms.
Previously, when crawlers triggered a Discourse::InvalidAccess exception, they would be served the full Ember SPA. The SPA is not optimized for crawling, and so this is likely to cause problems for sites. This issue is particularly problematic when user profiles are hidden from the public via the `hide_user_profiles_from_public` setting, because the crawler would end up being 'soft-redirected' to the homepage in the SPA.
Why this change?
In https://web.dev/articles/preconnect-and-dns-prefetch, it describes
how hinting to the browser to preconnect to domains which we will
eventually use the connection for can help improve the time it takes to
load a page.
We are putting this behind an experimental flag so that we can test and
profile this in a production environment.
What does this change introduce?
Introduce a hidden experimental `experimental_preconnect_link_header`
site setting which when enabled will add the `preconnect` and
`dns-prefetch` resource hints to the response headers for full page load
requests.
With the new admin sidebar restructure, we have a link to "Installed plugins". We would like to ensure that when the admin is searching for a plugin name like "akismet" or "automation" this link will be visible. Also when entering the plugins page, related plugins should be highlighted.
Alan Guo Xiang Tan
Natalie Tay
David Taylor
Jarek RadoszandLoïc Guitaut
chapoi
Régis Hanol
Penar Musaraj
Ted Johansson
Jeff Wong
Osama Sayegh
Angus McLeod
Martin Brennan
Krzysztof Kotlarek