From 73a1320d7264be2e8bfe80e28a871ccb3e6cc159 Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Thu, 18 Jun 2026 08:33:35 -0500 Subject: [PATCH 1/3] fix: derive Joomla element name with correct lowercase + type prefix AutoElementName() was using the manifest Name field verbatim, producing element names like "pkg_MokoSuiteBackup" instead of "pkg_mokosuitebackup". Joomla's updater matches by element+type+client_id in #__extensions, so the case mismatch made updates invisible. Changes: - Lowercase name and strip hyphens in AutoElementName() - Remove incorrect "plg_" prefix for plugins (Joomla plugins have no element prefix; the folder column determines the plugin group) Fixes #635 --- models/repo/repo_manifest.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/models/repo/repo_manifest.go b/models/repo/repo_manifest.go index 989342530c..48dde42b93 100644 --- a/models/repo/repo_manifest.go +++ b/models/repo/repo_manifest.go @@ -5,6 +5,7 @@ package repo import ( "context" + "strings" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/db" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/timeutil" @@ -60,10 +61,11 @@ func (RepoManifest) TableName() string { } // joomlaTypePrefix maps Joomla extension types to their element name prefixes. +// Plugins have no prefix in Joomla's #__extensions table — the element is just +// the lowercased name, and the folder column determines the plugin group. var joomlaTypePrefix = map[string]string{ "component": "com_", "module": "mod_", - "plugin": "plg_", "package": "pkg_", "template": "tpl_", "library": "lib_", @@ -71,14 +73,17 @@ var joomlaTypePrefix = map[string]string{ } // AutoElementName returns the auto-constructed Joomla element name (e.g. pkg_mokowaas). +// The name is lowercased and hyphens are stripped to match the convention Joomla +// uses in #__extensions.element (e.g. "MokoSuiteBackup" → "pkg_mokosuitebackup"). func (m *RepoManifest) AutoElementName() string { if m.Name == "" || m.PackageType == "" { return "" } + lower := strings.ToLower(strings.ReplaceAll(m.Name, "-", "")) if prefix, ok := joomlaTypePrefix[m.PackageType]; ok { - return prefix + m.Name + return prefix + lower } - return m.Name + return lower } // FullElementName returns the effective element name: override if set, otherwise auto-constructed. -- 2.52.0 From a83d2ee3bd01c2568e9a08c897a5aafd2b94ca9d Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Thu, 18 Jun 2026 09:14:13 -0500 Subject: [PATCH 2/3] fix: changelog element mismatch, platform gating, domain race condition Three fixes for the Joomla update server system: 1. changelog_xml.go: Resolve element name from manifest first (same priority as updates.xml) so changelog.xml and updates.xml emit matching values. Previously only checked the config table. 2. updateserver.go: Only serve Joomla XML when platform is joomla, both, or unset. Previously only blocked dolibarr, meaning WordPress/ PrestaShop/Drupal/WHMCS repos incorrectly served Joomla XML. 3. license_key.go: Wrap domain auto-association in db.WithTx to prevent TOCTOU race where concurrent requests from different domains could exceed MaxSites. Also removes a duplicate site-limit check that was unreachable dead code. --- models/updateserver/license_key.go | 137 +++++++++++++++-------------- routers/web/repo/changelog_xml.go | 27 ++++-- routers/web/repo/updateserver.go | 5 +- 3 files changed, 94 insertions(+), 75 deletions(-) diff --git a/models/updateserver/license_key.go b/models/updateserver/license_key.go index 3eac63278a..b3d1b6fa88 100644 --- a/models/updateserver/license_key.go +++ b/models/updateserver/license_key.go @@ -256,74 +256,8 @@ func ValidateLicenseKey(ctx context.Context, rawKey, domain string) (*LicenseKey // Domain restriction check — skip for internal/master keys. if domain != "" && !key.IsInternal { - now := timeutil.TimeStampNow() - - if key.DomainRestriction != "" { - // Domain restriction is set — enforce it. - allowed := false - for _, d := range strings.Split(key.DomainRestriction, ",") { - if strings.EqualFold(strings.TrimSpace(d), domain) { - allowed = true - break - } - } - if !allowed { - // Check if still within the domain lock grace period. - lockHours := pkg.DomainLockHours - if lockHours > 0 && key.FirstUsedUnix > 0 { - lockDeadline := key.FirstUsedUnix + timeutil.TimeStamp(int64(lockHours)*3600) - if now < lockDeadline { - // Grace period active — allow and auto-add this domain. - _ = updateDomainRestriction(ctx, key.ID, domain) - key.DomainRestriction = key.DomainRestriction + "," + domain - allowed = true - } - } - if !allowed { - return nil, nil, fmt.Errorf("domain not allowed for this license key") - } - } - } else { - // No domain restriction set — auto-associate domain. - maxSites := key.MaxSites - if maxSites == 0 { - maxSites = pkg.MaxSites - } - domainKnown, _ := IsDomainKnownForKey(ctx, key.ID, domain) - if !domainKnown { - if maxSites > 0 { - uniqueDomains, err := CountUniqueDomainsByKey(ctx, key.ID) - if err != nil { - return nil, nil, fmt.Errorf("failed to count domains: %w", err) - } - if uniqueDomains >= int64(maxSites) { - return nil, nil, fmt.Errorf("site limit reached (%d/%d)", uniqueDomains, maxSites) - } - } - _ = updateDomainRestriction(ctx, key.ID, domain) - if key.DomainRestriction == "" { - key.DomainRestriction = domain - } else { - key.DomainRestriction = key.DomainRestriction + "," + domain - } - } - } - - // Site limit check: use key's MaxSites, fall back to package default. - maxSites := key.MaxSites - if maxSites == 0 { - maxSites = pkg.MaxSites - } - if maxSites > 0 { - uniqueDomains, err := CountUniqueDomainsByKey(ctx, key.ID) - if err != nil { - return nil, nil, fmt.Errorf("failed to count domains: %w", err) - } - // Allow if this domain is already recorded, or if under the limit. - domainKnown, _ := IsDomainKnownForKey(ctx, key.ID, domain) - if !domainKnown && uniqueDomains >= int64(maxSites) { - return nil, nil, fmt.Errorf("site limit reached (%d/%d)", uniqueDomains, maxSites) - } + if err := validateAndAssociateDomain(ctx, key, pkg, domain); err != nil { + return nil, nil, err } } @@ -374,6 +308,73 @@ func ValidateLicenseKeyForRepo(ctx context.Context, rawKey, domain string, repoI return key, pkg, nil } +// validateAndAssociateDomain checks domain restrictions and auto-associates new +// domains within a transaction to prevent TOCTOU races on the MaxSites limit. +func validateAndAssociateDomain(ctx context.Context, key *LicenseKey, pkg *LicensePackage, domain string) error { + if key.DomainRestriction != "" { + // Domain restriction is set — enforce it. + allowed := false + for _, d := range strings.Split(key.DomainRestriction, ",") { + if strings.EqualFold(strings.TrimSpace(d), domain) { + allowed = true + break + } + } + if !allowed { + // Check if still within the domain lock grace period. + now := timeutil.TimeStampNow() + lockHours := pkg.DomainLockHours + if lockHours > 0 && key.FirstUsedUnix > 0 { + lockDeadline := key.FirstUsedUnix + timeutil.TimeStamp(int64(lockHours)*3600) + if now < lockDeadline { + // Grace period active — allow and auto-add this domain. + _ = updateDomainRestriction(ctx, key.ID, domain) + key.DomainRestriction = key.DomainRestriction + "," + domain + allowed = true + } + } + if !allowed { + return fmt.Errorf("domain not allowed for this license key") + } + } + return nil + } + + // No domain restriction set — auto-associate domain within a transaction + // so the count check and insert are atomic (prevents exceeding MaxSites). + maxSites := key.MaxSites + if maxSites == 0 { + maxSites = pkg.MaxSites + } + + return db.WithTx(ctx, func(txCtx context.Context) error { + domainKnown, _ := IsDomainKnownForKey(txCtx, key.ID, domain) + if domainKnown { + return nil // already associated, nothing to do + } + + if maxSites > 0 { + uniqueDomains, err := CountUniqueDomainsByKey(txCtx, key.ID) + if err != nil { + return fmt.Errorf("failed to count domains: %w", err) + } + if uniqueDomains >= int64(maxSites) { + return fmt.Errorf("site limit reached (%d/%d)", uniqueDomains, maxSites) + } + } + + if err := updateDomainRestriction(txCtx, key.ID, domain); err != nil { + return err + } + if key.DomainRestriction == "" { + key.DomainRestriction = domain + } else { + key.DomainRestriction = key.DomainRestriction + "," + domain + } + return nil + }) +} + // updateDomainRestriction appends a domain to a key's DomainRestriction field in the DB. func updateDomainRestriction(ctx context.Context, keyID int64, domain string) error { key, err := GetLicenseKeyByID(ctx, keyID) diff --git a/routers/web/repo/changelog_xml.go b/routers/web/repo/changelog_xml.go index db33a8a26d..c7d9c4117d 100644 --- a/routers/web/repo/changelog_xml.go +++ b/routers/web/repo/changelog_xml.go @@ -10,8 +10,9 @@ import ( "strings" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/db" - updateserver_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/updateserver" repo_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/repo" + updateserver_model "code.mokoconsulting.tech/MokoConsulting/MokoGitea/models/updateserver" + "code.mokoconsulting.tech/MokoConsulting/MokoGitea/modules/log" "code.mokoconsulting.tech/MokoConsulting/MokoGitea/services/context" ) @@ -57,15 +58,31 @@ func ServeChangelogXML(ctx *context.Context) { return } - // Get extension metadata for element name and type. - cfg := updateserver_model.GetEffectiveConfig(ctx, repo.OwnerID, repo.ID) + // Resolve element name and type using the same priority as updates.xml: + // manifest first, then config table fallback, then repo-derived default. element := strings.ToLower(repo.Name) extType := "component" + + manifest, err := repo_model.GetRepoManifest(ctx, repo.ID) + if err != nil { + log.Error("ServeChangelogXML: GetRepoManifest for repo %d: %v", repo.ID, err) + } + if manifest != nil { + if elem := manifest.FullElementName(); elem != "" { + element = elem + } + if manifest.PackageType != "" { + extType = manifest.PackageType + } + } + + // Config table as fallback for repos without a manifest. + cfg := updateserver_model.GetEffectiveConfig(ctx, repo.OwnerID, repo.ID) if cfg != nil { - if cfg.ExtensionName != "" { + if element == strings.ToLower(repo.Name) && cfg.ExtensionName != "" { element = cfg.ExtensionName } - if cfg.ExtensionType != "" { + if extType == "component" && cfg.ExtensionType != "" { extType = cfg.ExtensionType } } diff --git a/routers/web/repo/updateserver.go b/routers/web/repo/updateserver.go index 5ff4356a75..b55bc7fcec 100644 --- a/routers/web/repo/updateserver.go +++ b/routers/web/repo/updateserver.go @@ -78,9 +78,10 @@ func validateUpdateKey(ctx *context.Context) (allowedChannels []string, ok bool, // ServeUpdatesXML generates and serves a Joomla-compatible updates.xml // from the repository's releases. func ServeUpdatesXML(ctx *context.Context) { - // Block if platform doesn't include joomla. + // Block if platform is set to a non-Joomla value. + // Empty/unset defaults to joomla for backwards compatibility. platform := ctx.Data["RepoUpdatePlatform"] - if platform == "dolibarr" { + if platform != nil && platform != "" && platform != "joomla" && platform != "both" { ctx.NotFound(nil) return } -- 2.52.0 From 08f6454dd2bd8cb67c14e7b498acfdcd3af47c35 Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Thu, 18 Jun 2026 09:51:33 -0500 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20address=20PR=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20error=20handling,=20type=20safety,=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes: - Propagate updateDomainRestriction error in grace-period path instead of silently discarding it (was same bug class as the TOCTOU fix) - Propagate IsDomainKnownForKey error inside transaction — discarding it defeated the atomicity guarantee - Wrap updateDomainRestriction error with context message - Use boolean flags for changelog manifest fallback instead of fragile sentinel comparison against strings.ToLower(repo.Name) - Type-assert ctx.Data["RepoUpdatePlatform"] to string instead of comparing interface{} values - Use log.Warn instead of log.Error for manifest fallback (intentional degradation, not a failure) - Clarify comments: doc comment scope, hyphen removal wording --- models/repo/repo_manifest.go | 8 ++++---- models/updateserver/license_key.go | 15 +++++++++++---- routers/web/repo/changelog_xml.go | 14 +++++++++----- routers/web/repo/updateserver.go | 4 ++-- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/models/repo/repo_manifest.go b/models/repo/repo_manifest.go index 48dde42b93..67ed25345c 100644 --- a/models/repo/repo_manifest.go +++ b/models/repo/repo_manifest.go @@ -61,8 +61,8 @@ func (RepoManifest) TableName() string { } // joomlaTypePrefix maps Joomla extension types to their element name prefixes. -// Plugins have no prefix in Joomla's #__extensions table — the element is just -// the lowercased name, and the folder column determines the plugin group. +// Plugins have no prefix in Joomla's #__extensions table — the element is the +// lowercased, hyphen-free name, and the folder column determines the plugin group. var joomlaTypePrefix = map[string]string{ "component": "com_", "module": "mod_", @@ -73,8 +73,8 @@ var joomlaTypePrefix = map[string]string{ } // AutoElementName returns the auto-constructed Joomla element name (e.g. pkg_mokowaas). -// The name is lowercased and hyphens are stripped to match the convention Joomla -// uses in #__extensions.element (e.g. "MokoSuiteBackup" → "pkg_mokosuitebackup"). +// The name is lowercased and hyphens are removed to produce clean element names +// for the #__extensions.element column (e.g. "MokoSuiteBackup" → "pkg_mokosuitebackup"). func (m *RepoManifest) AutoElementName() string { if m.Name == "" || m.PackageType == "" { return "" diff --git a/models/updateserver/license_key.go b/models/updateserver/license_key.go index b3d1b6fa88..a53d934fb8 100644 --- a/models/updateserver/license_key.go +++ b/models/updateserver/license_key.go @@ -309,7 +309,9 @@ func ValidateLicenseKeyForRepo(ctx context.Context, rawKey, domain string, repoI } // validateAndAssociateDomain checks domain restrictions and auto-associates new -// domains within a transaction to prevent TOCTOU races on the MaxSites limit. +// domains. The auto-associate path (no existing restriction) runs inside a +// transaction to prevent TOCTOU races on the MaxSites limit. The grace-period +// path (existing restriction, lock window open) also propagates DB errors. func validateAndAssociateDomain(ctx context.Context, key *LicenseKey, pkg *LicensePackage, domain string) error { if key.DomainRestriction != "" { // Domain restriction is set — enforce it. @@ -328,7 +330,9 @@ func validateAndAssociateDomain(ctx context.Context, key *LicenseKey, pkg *Licen lockDeadline := key.FirstUsedUnix + timeutil.TimeStamp(int64(lockHours)*3600) if now < lockDeadline { // Grace period active — allow and auto-add this domain. - _ = updateDomainRestriction(ctx, key.ID, domain) + if err := updateDomainRestriction(ctx, key.ID, domain); err != nil { + return fmt.Errorf("failed to auto-add domain during grace period: %w", err) + } key.DomainRestriction = key.DomainRestriction + "," + domain allowed = true } @@ -348,7 +352,10 @@ func validateAndAssociateDomain(ctx context.Context, key *LicenseKey, pkg *Licen } return db.WithTx(ctx, func(txCtx context.Context) error { - domainKnown, _ := IsDomainKnownForKey(txCtx, key.ID, domain) + domainKnown, err := IsDomainKnownForKey(txCtx, key.ID, domain) + if err != nil { + return fmt.Errorf("failed to check domain association: %w", err) + } if domainKnown { return nil // already associated, nothing to do } @@ -364,7 +371,7 @@ func validateAndAssociateDomain(ctx context.Context, key *LicenseKey, pkg *Licen } if err := updateDomainRestriction(txCtx, key.ID, domain); err != nil { - return err + return fmt.Errorf("failed to update domain restriction: %w", err) } if key.DomainRestriction == "" { key.DomainRestriction = domain diff --git a/routers/web/repo/changelog_xml.go b/routers/web/repo/changelog_xml.go index c7d9c4117d..b0e83530db 100644 --- a/routers/web/repo/changelog_xml.go +++ b/routers/web/repo/changelog_xml.go @@ -58,31 +58,35 @@ func ServeChangelogXML(ctx *context.Context) { return } - // Resolve element name and type using the same priority as updates.xml: + // Resolve element name and type: // manifest first, then config table fallback, then repo-derived default. element := strings.ToLower(repo.Name) extType := "component" + elementFromManifest := false + extTypeFromManifest := false manifest, err := repo_model.GetRepoManifest(ctx, repo.ID) if err != nil { - log.Error("ServeChangelogXML: GetRepoManifest for repo %d: %v", repo.ID, err) + log.Warn("ServeChangelogXML: GetRepoManifest for repo %d: %v", repo.ID, err) } if manifest != nil { if elem := manifest.FullElementName(); elem != "" { element = elem + elementFromManifest = true } if manifest.PackageType != "" { extType = manifest.PackageType + extTypeFromManifest = true } } - // Config table as fallback for repos without a manifest. + // Config table fallback: apply only when the manifest did not provide a value. cfg := updateserver_model.GetEffectiveConfig(ctx, repo.OwnerID, repo.ID) if cfg != nil { - if element == strings.ToLower(repo.Name) && cfg.ExtensionName != "" { + if !elementFromManifest && cfg.ExtensionName != "" { element = cfg.ExtensionName } - if extType == "component" && cfg.ExtensionType != "" { + if !extTypeFromManifest && cfg.ExtensionType != "" { extType = cfg.ExtensionType } } diff --git a/routers/web/repo/updateserver.go b/routers/web/repo/updateserver.go index b55bc7fcec..afb9f989de 100644 --- a/routers/web/repo/updateserver.go +++ b/routers/web/repo/updateserver.go @@ -80,8 +80,8 @@ func validateUpdateKey(ctx *context.Context) (allowedChannels []string, ok bool, func ServeUpdatesXML(ctx *context.Context) { // Block if platform is set to a non-Joomla value. // Empty/unset defaults to joomla for backwards compatibility. - platform := ctx.Data["RepoUpdatePlatform"] - if platform != nil && platform != "" && platform != "joomla" && platform != "both" { + platform, _ := ctx.Data["RepoUpdatePlatform"].(string) + if platform != "" && platform != "joomla" && platform != "both" { ctx.NotFound(nil) return } -- 2.52.0