Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
7d7eb85
[#71630] POC: leverage friendly_id :history for persistent project id…
akabiru Mar 10, 2026
3fec880
[#71630] Fix redirect_if_historical_project_identifier to pass throug…
akabiru Mar 11, 2026
01cedde
Validate for historic identifiers
judithroth Mar 11, 2026
7c7aa05
Fix spec
judithroth Mar 11, 2026
a40e47e
WIP: Temporarily remove failing spec to get a feature spec run
judithroth Mar 11, 2026
4d3eaec
[#71630] Fix FriendlyId::Slugged#unset_slug_if_invalid clearing ident…
akabiru Mar 11, 2026
7ca47c5
[#71630] Fix double error and clarify identifier uniqueness + histori…
akabiru Mar 12, 2026
d07f13f
[#71630] Test that a project can revert to its own historical identifier
akabiru Mar 12, 2026
db8ade3
Use error exclusion instead of query as the safer method
akabiru Mar 12, 2026
054b7bb
[#71630] Redirect stale project identifiers in all controller loaders
akabiru Mar 12, 2026
0f24b9f
[#71630] Use friendly_id? instead of regex for numeric ID guard
akabiru Mar 12, 2026
0aecd0d
[#71630] Remove stale-identifier redirect from load_and_authorize_opt…
akabiru Mar 12, 2026
39c6ff7
[#71630] Add migration to initialize friendly_id_slugs with current d…
judithroth Mar 12, 2026
4ca48e0
[#72917] Improve migration and add migration spec
judithroth Mar 12, 2026
abc9aae
[#72917] Remove unnecessary require statements
judithroth Mar 12, 2026
a1f88c6
[#71630] Fix turbo streams that are not redirectable
judithroth Mar 12, 2026
1713db5
[#71630] Refine wording
judithroth Mar 13, 2026
51d5ec6
[#71630] Redirect historic identifiers in the API
judithroth Mar 13, 2026
c42a6d5
[#71630] Add even more redirects
judithroth Mar 13, 2026
50b2fdf
[#71630] Rename file for consistency
judithroth Mar 13, 2026
ba09157
[#71630] Also update Bim::Bcf::API::V2_1::ProjectsAPI
judithroth Mar 13, 2026
391e9ec
[#71630] Ensure redirects without security issues
judithroth Mar 13, 2026
b69328b
[#71630] Use bigint for ID column in friendly_id_slugs
judithroth Mar 13, 2026
f4d42dc
[#71630] Ensure redirects in the API without security issues
judithroth Mar 16, 2026
c9fb8f8
fix: properly detect HTML content type in historic identifier specs
thykel Mar 17, 2026
e6e67d8
fix: extend the path replacement regex to include identifier dashes
thykel Mar 17, 2026
8413818
fix: add a missing feature flag setting for rb sprints controller test
thykel Mar 17, 2026
bf4fe67
refactor: Appease the Rubocop complexity scanner
thykel Mar 17, 2026
c9da7de
Apply suggestion from @thykel
judithroth Mar 18, 2026
3a8c642
Apply suggestion from @thykel
judithroth Mar 18, 2026
44f00d8
Potential fix for pull request finding
judithroth Mar 18, 2026
babdc34
Potential fix for pull request finding
judithroth Mar 18, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -246,17 +246,47 @@
# Note: find() is Project.friendly.find()
def find_project
@project = Project.visible.find(params[:id])
redirect_if_historical_project_identifier(:id)
end

# Find project of id params[:project_id]
# Note: find() is Project.friendly.find()
def find_project_by_project_id
@project = Project.visible.find(params[:project_id])
redirect_if_historical_project_identifier(:project_id)
end

# Redirects to the project's current identifier if the request used a historical slug.
# friendly_id's :history extension records old identifiers in friendly_id_slugs; if the
# param is a slug (param.friendly_id? is truthy) that no longer matches the current
# identifier, it must be a historical slug. Numeric IDs are passed through without redirect.
# Only redirects HTML requests; turbo_stream and other formats are left as-is.
def redirect_if_historical_project_identifier(identifier_param_key)
param = params[identifier_param_key]
return unless request.get? && request.format.symbol == :html && param.friendly_id? && param != @project.identifier

redirect_to canonical_project_url(identifier_param_key), status: :moved_permanently
end

def canonical_project_url(identifier_param_key)
# Reconstruct the URL from path + query parameters, and prevent user-supplied
# options such as :host or :protocol from influencing the redirect target.
safe_path_params = request.path_parameters.symbolize_keys
safe_query_params = request.query_parameters.symbolize_keys

safe_path_params[identifier_param_key] = @project.identifier
safe_query_params.except!(:host, :protocol, :subdomain, :domain, :port)

url_for(safe_path_params.merge(safe_query_params).merge(only_path: true))
end

# Find project by project_id if given
def find_optional_project
@project = Project.visible.find(params[:project_id]) if params[:project_id].present?
return if params[:project_id].blank?

Check notice on line 286 in app/controllers/application_controller.rb

View workflow job for this annotation

GitHub Actions / rubocop

[rubocop] app/controllers/application_controller.rb#L286 <Layout/TrailingWhitespace>

Trailing whitespace detected.
Raw output
app/controllers/application_controller.rb:286:1: C: Layout/TrailingWhitespace: Trailing whitespace detected.
@project = Project.visible.find(params[:project_id])
redirect_if_historical_project_identifier(:project_id)
nil if performed?
rescue ActiveRecord::RecordNotFound
render_404
end
Expand Down
1 change: 1 addition & 0 deletions app/controllers/categories_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,5 +87,6 @@ def find_category_and_project

def find_project
@project = Project.visible.find(params[:project_id])
redirect_if_historical_project_identifier(:project_id)
end
end
3 changes: 3 additions & 0 deletions app/controllers/messages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ def quote

def find_project_and_forum
@project = Project.visible.find(params[:project_id])
redirect_if_historical_project_identifier(:project_id)
return if performed?

@forum = @project.forums.find(params[:forum_id])
end

Expand Down
3 changes: 3 additions & 0 deletions app/controllers/news/comments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ def find_comment

def find_news_and_project
@project = Project.visible.find(params[:project_id])
redirect_if_historical_project_identifier(:project_id)
return if performed?

@news = @project.news.visible.find(params[:news_id])
end
end
6 changes: 4 additions & 2 deletions app/controllers/projects/archive_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,11 @@ def dialog
private

def find_project_including_archived
# The visible scope filters out archived projects, but here we want to explicitly unarchive them.
# The contracts do proper permission checks, so we can skip the visible scope here.
# Skips the visible scope so archived projects are accessible.
# The contracts do proper permission checks. Project admins (regular users with admin role)
# may reach these actions via an old identifier, so redirect if needed.
@project = Project.find(params[:project_id])
redirect_if_historical_project_identifier(:project_id)
end

def change_status_action(status)
Expand Down
5 changes: 3 additions & 2 deletions app/controllers/projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,10 @@ def export_project_initiation_pdf
private

def find_project_including_archived
# The actions that use this method are only accessible to admins, so we can show them archived projects as well and
# can skip the visible scope here.
# Skips the visible scope so archived projects are accessible.
# Project admins (regular users with admin role) may reach these actions via an old identifier.
@project = Project.find(params[:id])
redirect_if_historical_project_identifier(:id)
end

def from_template? = @template.present?
Expand Down
5 changes: 4 additions & 1 deletion app/controllers/users/invite_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@
end

def set_project
@project = Project.find(params[:project_id]) if params[:project_id].present?
return if params[:project_id].blank?

Check notice on line 138 in app/controllers/users/invite_controller.rb

View workflow job for this annotation

GitHub Actions / rubocop

[rubocop] app/controllers/users/invite_controller.rb#L138 <Layout/TrailingWhitespace>

Trailing whitespace detected.
Raw output
app/controllers/users/invite_controller.rb:138:1: C: Layout/TrailingWhitespace: Trailing whitespace detected.
@project = Project.find(params[:project_id])
redirect_if_historical_project_identifier(:project_id)
end

def dialog_title
Expand Down
1 change: 1 addition & 0 deletions app/controllers/versions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ def redirect_back_or_version_settings

def find_project
@project = Project.visible.find(params[:project_id])
redirect_if_historical_project_identifier(:project_id)
end

def retrieve_selected_type_ids(selectable_types, default_types = nil)
Expand Down
3 changes: 3 additions & 0 deletions app/controllers/wiki_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,9 @@ def wiki_page_title

def find_wiki
@project = Project.visible.find(params[:project_id])
redirect_if_historical_project_identifier(:project_id)
return if performed?

@wiki = @project.wiki
render_404 unless @wiki
end
Expand Down
34 changes: 32 additions & 2 deletions app/models/project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -202,19 +202,32 @@ def validation_context

validates :identifier,
presence: true,
uniqueness: { case_sensitive: true },
uniqueness: { case_sensitive: true }, # currently owned by another project
length: { maximum: IDENTIFIER_MAX_LENGTH },
exclusion: RESERVED_IDENTIFIERS,
if: ->(p) { p.persisted? || p.identifier.present? }

# Complements the uniqueness validation above: once an identifier has been used by a
# project, it remains reserved for that project even after the project moves to a new
# identifier. This prevents another project from claiming a "retired" identifier.
# Powered by the friendly_id :history extension — see the friendly_id declaration below.
validate :identifier_not_historically_reserved, if: ->(p) { p.identifier_changed? }

# Contains only a-z, 0-9, dashes and underscores but cannot consist of numbers only as it would clash with the id.
validates :identifier,
format: { with: /\A(?!^\d+\z)[a-z0-9\-_]+\z/ },
if: ->(p) { p.identifier_changed? && p.identifier.present? }

validates_associated :repository, :wiki

friendly_id :identifier, use: :finders
friendly_id :identifier, use: %i[finders history], slug_column: :identifier

# FriendlyId::Slugged adds after_validation :unset_slug_if_invalid, which reverts the
# slug column to its previous value when validation fails. With slug_column: :identifier,
# this would reset a manually-set identifier back to nil on new records. Since the
# identifier is managed by acts_as_url and user input (not FriendlyId's slug generator),
# we disable this behaviour entirely.
def unset_slug_if_invalid; end

scopes :activated_in_storage,
:allowed_to,
Expand Down Expand Up @@ -356,4 +369,21 @@ def module_disabled(disabled_module)
OpenProject::Events::MODULE_DISABLED, disabled_module:
)
end

private

# Checks friendly_id_slugs for any project that previously used this identifier and
# has since changed it. Excludes projects that still hold the identifier as their
# current one — those are already caught by the uniqueness validation above, and
# including them here would produce a duplicate error.
def identifier_not_historically_reserved
return if errors.any? { |error| error.attribute == :identifier && error.type == :taken }

already_existing = FriendlyId::Slug
.where(slug: identifier, sluggable_type: self.class.to_s)
.where.not(sluggable_id: id)
.exists?

errors.add(:identifier, :taken) if already_existing
end
end
48 changes: 48 additions & 0 deletions db/migrate/20260311000000_create_friendly_id_slugs.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

class CreateFriendlyIdSlugs < ActiveRecord::Migration[8.1]
def change
create_table :friendly_id_slugs do |t|
t.string :slug, null: false
t.bigint :sluggable_id, null: false
t.string :sluggable_type, limit: 50
t.string :scope
t.datetime :created_at
end

add_index :friendly_id_slugs, %i[sluggable_type sluggable_id]
add_index :friendly_id_slugs, %i[slug sluggable_type],
length: { slug: 140, sluggable_type: 50 }
add_index :friendly_id_slugs, %i[slug sluggable_type scope],
length: { slug: 70, sluggable_type: 50, scope: 70 },
unique: true
end
end
51 changes: 51 additions & 0 deletions db/migrate/20260312121938_initialize_historic_identifiers.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

class InitializeHistoricIdentifiers < ActiveRecord::Migration[8.1]
def up
execute <<~SQL.squish
INSERT INTO friendly_id_slugs (slug, sluggable_id, sluggable_type, scope, created_at)
SELECT p.identifier, p.id, 'Project', NULL, NOW()
FROM projects p
WHERE p.identifier IS NOT NULL
AND NOT EXISTS (
SELECT 1 FROM friendly_id_slugs fis
WHERE fis.slug = p.identifier
AND fis.sluggable_id = p.id
AND fis.sluggable_type = 'Project'
AND fis.scope IS NULL
)
SQL
end

def down
# nothing to do / not possible
end
end
79 changes: 79 additions & 0 deletions lib/api/helpers/historical_identifier_redirect.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module API
module Helpers
module HistoricalIdentifierRedirect
# Redirects API requests using a historical project identifier to the canonical URL
# with the project's current identifier.
#
# Returns a 301 Moved Permanently response when the request uses a historical
# (retired) project identifier. This ensures API responses always use canonical URLs.
#
# @param identifier_param [Symbol] The route parameter name (e.g., :id, :project, :of)
# @param project [Project] The loaded project instance
#
# @example In a Grape API endpoint
# route_param :id do
# after_validation do
# helpers ::API::Helpers::HistoricalIdentifierRedirect
# @project = Project.find(params[:id])
# redirect_if_historical_identifier(:id, @project)
# end
# end
def redirect_if_historical_identifier(identifier_param, project)
Copy link
Copy Markdown
Contributor

@thykel thykel Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@judithroth This helper is currently rather raw because Grape doesn't have the same URL construction capabilities as Rails, right?

I might have some good news -- it looks like we have a helper that allows us to construct URLs neatly within Grape as well. 🙂 => quick example here

Lemme know what you think -- at the very least, it looks like it's making tests pass.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that solution very much - thanks for suggesting it!

param_value = params[identifier_param]
return unless request.get? && param_value.friendly_id? && param_value != project.identifier

redirect canonical_identifier_path(identifier_param, param_value, project), permanent: true
end

def canonical_identifier_path(identifier_param, param_value, project)
# Replace the old identifier in the path.
# This prevents Host header injection and open redirect attacks.
new_path = request.path.sub(
%r{(/)#{Regexp.escape(param_value)}(/|-|$)},
"\\1#{project.identifier}\\2"
)

# Replace the old identifier in query parameters if present
if request.query_string.present?
new_query_string = request.query_string.gsub(
/(\A|&)#{Regexp.escape(identifier_param.to_s)}=#{Regexp.escape(param_value)}(&|\z)/,
"\\1#{identifier_param}=#{CGI.escape(project.identifier)}\\2"
)
new_path += "?#{new_query_string}"
end

new_path
end
end
end
end
11 changes: 10 additions & 1 deletion lib/api/v3/projects/available_parents_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,26 @@ module V3
module Projects
class AvailableParentsAPI < ::API::OpenProjectAPI
resource :available_parent_projects do
helpers ::API::Helpers::HistoricalIdentifierRedirect

after_validation do
authorize_globally(:add_project) do
authorize_in_any_project(%i[add_subprojects edit_project])
end

# Handle redirect for historical identifier in :of query param
if params[:of]
@of_project = Project.find(params[:of])
redirect_if_historical_identifier(:of, @of_project)
end
Comment on lines 38 to +47
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should look into this, probably not using visible was not intentional but we should check.

end

get &::API::V3::Utilities::Endpoints::SqlFallbackedIndex.new(
model: Project,
scope: -> do
project = if params[:of]
Project.find(params[:of])
# Reuse @of_project if available (already found in after_validation)
@of_project || Project.find(params[:of])
else
Project.new(workspace_type: params[:workspace_type] || "project")
end
Expand Down
Loading
Loading