[1.0.x] TMGMT LATW Cloud (AI Translation)

Reviewed 1.0.x at 8111510. I read the signing and crypto path and the endpoint surface, ran phpcs (Drupal, DrupalPractice), checked licensing and third-party assets, and checked the Drupal 10/11 compatibility claims against the code.

What is already right

Worth stating specifically, because this is a better-prepared application than most:

• The canonical string binds method, path, timestamp, nonce and a SHA-256 of the body, so none of them can be swapped. hash_equals() for the comparison, random_bytes() for nonces, PKCE S256 for pairing.
• The signed path is relative to the API base, so a subdirectory install or a proxy rewriting the prefix cannot change what was signed. That is a thoughtful detail.
• setWithExpireIfNotExists() is genuinely atomic, so two concurrent requests carrying one nonce cannot both win. NONCE_TTL of 600 against a WINDOW of plus/minus 300 is the correct relationship: the nonce outlives every moment its signature is still valid.
• Secret at rest: HKDF-SHA256 from the hash salt with its own info string, AES-256-GCM, a fresh 12-byte IV per encryption, tag verified on the way back, and it fails closed to an empty string rather than throwing. The docblock about a changed salt is accurate.
• Version compatibility is actually implemented rather than only declared: #[LegacyHook] wrappers, #[LegacyRequirementsHook] with a class_exists(RequirementSeverity::class) guard so 11.2 does not double-report, and an explicit service registration for the hook class because Drupal 10 does not register them itself.
• No LICENSE.txt committed, GPL-2.0-or-later in composer.json, no bundled libraries, stock gitlab_templates CI, config/schema present, and phpcs clean across all PHP.

1. Raw exception messages are returned on a path reachable without authentication

In src/EventSubscriber/CloudApiExceptionSubscriber.php:

$this->logger->error('LATW Cloud request to @path failed: @error (@where)', [...]);
$error = new CloudException('internal_error', $exception->getMessage(), 500);

CloudException::toBody() emits 'message' => $this->getMessage(), so that string reaches the caller in the JSON body. The default arm of the HttpExceptionInterface match just above does the same.

The subscriber decides whether to handle a request on the path prefix alone, before and independently of SignatureAccessCheck, so the caller at that point has not necessarily authenticated. Connection::get() necessarily runs ahead of any signature check, because it has to in order to find the key, and it touches State and then Settings::getHashSalt(). A DatabaseExceptionWrapper there carries the failing SQL and the table prefix in its message; a missing hash salt raises a RuntimeException naming settings.php.

The detail is already logged on the line above, including file and line, so returning it as well adds disclosure without adding diagnostic value. A fixed string for internal_error, and for the default arm, would close it.

This is the only thing I would hold the application on.

2. $is_syncing is accepted and then ignored

tmgmt_latw_uninstall(bool $is_syncing) makes an unconditional outbound HTTP call to LATW Cloud. The parameter exists so a module can skip side effects when the uninstall is replaying configuration rather than expressing an intent, and here the side effect is a network call to a third party, which also means an uninstall during a config import blocks on an external timeout.

Not necessarily wrong, since you may well want the panel told either way. But it reads as an oversight rather than a decision, so: was it one? If the call is wanted during sync, a short comment saying so would settle it for the next reader.

3. Worth one sentence in the README

Scope is bundles with Content Translation enabled, which is a real and defensible boundary. Within it, ContentSource::countByType() filters on the default-translation column and not on published status, so unpublished and in-moderation sources are listed and translatable. That matches how TMGMT itself behaves and I am not asking for a change. But an administrator switching this on is sending unpublished content to a third party, and the README is the right place to say so plainly.

Two things that are not problems, recorded so the next reviewer does not chase them:

• phpcs reports 16 errors in js/connection.js (null to NULL, spacing around -). These are false positives from PHP-oriented sniffs reading JavaScript: core's own misc/drupal.js trips 28 of the same and misc/ajax.js trips 106. Drupal.org lints JavaScript with ESLint, and lowercase null, true and false are correct in JS. No change wanted.
• src/Hook/TmgmtLatwRequirements.php and src/Install/Requirements/TmgmtLatwRequirements.php look like a duplicate pair but are the correct Drupal 11.2 split, the runtime hook versus InstallRequirementsInterface. Correct as written.

Setting to Needs work for the first point only.

Thank you for applying!

Before giving links helpful to understand how the review process works, what to expect from a review, and what to do to avoid a review takes more time than needed, I would like to thank all the reviewers for the work they do.
These applications are volunters-driven, which also means it is not possible to predict when an application will be marked fixed and the applicant will get the permission to opt projects into security advisory policy. While we aim to make an application as quick as possible, it is also important for us that more people review the project used for an application. In this way, we make sure applications do not miss some important points that should be instead reported.
Applications are not meant to be complete debugging sessions that eliminate every existing bug, though. I apologize if sometimes applications seem to go into too-detailed reviews.

Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should enable GitLab CI for the project and fix the PHP_CodeSniffer errors/warnings it reports.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status will not be changed by this application; once this application is closed, you will be able to change the project status from Not covered to Opt into security advisory coverage. This is possible only 14 days after the project is created.
  
  Keep in mind that once the project is opted into security advisory coverage, only Security Team members may change coverage.
• Only the person who created the application will get the permission to opt projects into security advisory coverage. No other person will get the same permission from the same application; that applies also to co-maintainers/maintainers of the project used for the application.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications, Application workflow, What to cover in an application review, and Tools to use for reviews.

The important notes are the following.

• It is preferable to wait for a project moderator before posting the first comment on newly created applications. Project moderators will do some preliminary checks that are necessary before any change on the project files is suggested.
• For the tools to use for reviewing a project, see Tools to use for reviews.
• It may be best to have the applicant fix things before further reviews and avoid doing a review when another review is pending.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues.

FILE: tmgmt_latw.libraries.yml

version: VERSION

VERSION is only used by Drupal core modules. Contributed modules should use a literal string that does not change with the Drupal core version a site is using.

I read the 1.0.x branch at commit 3f1e219 and checked it against the review in #2 and the follow-up in #6. I did not run the tests or the pipeline.

The blocking item from #2 is resolved. CloudApiExceptionSubscriber::onException() now sends only CloudException messages and a fixed string for any other throwable, and the detail goes to the log. The same pattern is used in SiteApi when saving translations. SignatureAccessCheck runs before any endpoint, verifies the HMAC with hash_equals(), rejects timestamps outside the 300 second window, and only stores the nonce after the signature has passed, so an unauthenticated caller cannot fill the nonce store. The $is_syncing handling in tmgmt_latw_uninstall() and the literal 1.0.0 library version are also in place.

A few small points, none blocking.

SignatureAccessCheck::NONCE_TTL is 600 and the validity window is 300 seconds either side of now, so a request stamped 300 seconds ahead can be replayed until exactly 600 seconds after first use. The docblock says the nonce is kept "a little longer" than a signature is valid, but it is equal at the boundary. A small margin (for example Signer::WINDOW * 2 + 60) would match the comment.
The nonce is consumed inside an access checker. That works here because the check runs once per request, but access checks are not meant to have side effects, so a short comment saying so would help the next maintainer.
unknown_key and invalid_signature are different answers, so an unauthenticated caller can learn whether a key id matches. The key id is not a secret, so I would not change it, but it may be worth a line in the README.
tmgmt_latw_uninstall() calls LATW Cloud with a 30 second timeout when the module is uninstalled from the UI or Drush. If the service is slow, the uninstall waits. It is caught and logged, so it is safe, only slow.

I see no blocking concerns in the code I read.