[1.0.x] Cybersource SOP

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.

• For the purposes of this application, it is not necessary to create releases or pre-releases. It is better to make commits only on a branch, and possibly in the same branch used from the start.
• 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.
• Reviewers should show the output of a CLI tool only once per application.
• It may be best to have the applicant fix things before further review.

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.

It appears that you may have used AI tools in this application, module code, and even for the review bonus comments.

If that is correct, please read Drupal's AI contribution policy carefully and ensure that you have followed it in all cases: https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

As a side note, to use the review bonus, it is necessary to use the PAreview: review bonus issue tag.
Using the review bonus is probably not necessary, since this is a recent application and reviewers will probably review first the last created applications.

FILE: cybersource_sop.module

A new module that aims to be compatible with latest Drupal releases is expected to implement hooks as class methods as described in Support for object oriented hook implementations using autowired services.

As a side note, in a comment that changes the status to Needs review, it is not necessary to describe what it has been done. Reviewers assume that the project has been changed following the review, if a comment does not state I did not do this because [...].

I reviewed this on a fresh Drupal 11.4 with Commerce 3 install and looked at the security-critical paths properly rather than just reading them.

Suite's green here (38 tests, 525 assertions), phpcs clean against your own ruleset, phpstan level 6.

I dropped in a test credentials file with a known secret and fired forged and tampered replies at the return route to check the project-page claims hold. They do. An unsigned reply, a forged signature or one that leaves a critical field out of signed_field_names all get a 403 and record nothing. A signed reply whose amount doesn't match the order is refused, and the recorded amount comes from the order rather than the POST. One signed for a different order and POSTed here is refused on the reference check. A signed decline lets the customer retry with nothing recorded. A signed ACCEPT records a single authorisation, and replaying it leaves one payment - every attempt logged, no card data. The card fields are id-only and the JS has no fetch/XHR/storage, so card data really does go straight to Cybersource. I also tried the outbound side and the state machine - tampering the signed set, dropping signed fields, replay, a double-submit race - and it held, since the signing partition is itself signed and the order total is authoritative.

A few things, none a blocker:

1. You write the audit-log row before checking the signature - authenticateReply() calls logResponse() on the signature-invalid branch, and the route's open (correctly - it's HMAC-authenticated, not session-authenticated). So an unauthenticated POST for an order on this gateway still writes a commerce_log row before it's rejected; three junk POSTs here were all 403 with no payment but each still added a row. Minor, and it falls out of logging every attempt by design, but you might want the write behind the signature check, or the route flooded.
2. Two removed-in-Drupal-13 deprecations. cybersource_sop_requirements() has no #[LegacyRequirementsHook] attribute, so it trips on 11.3+ (I saw it in your own kernel run) - adding that, same BC shape as your hook_help conversion, sorts it and stays 10.3-safe since the attribute's only read via reflection. And the gateway still uses the @CommercePaymentGateway annotation rather than the attribute, a straight swap since Commerce 3 has it.
3. The amount's signed with number_format(..., 2), fixed at two decimals. Nothing stops a store using a currency that isn't two-decimal (JPY none, KWD three), and the signed amount is then the wrong precision. Not a pricing risk, since the recorded payment always uses the order total, but it'd break checkout for three-decimal currencies where the return-side check rejects the truncated value. Deriving the decimals from the currency covers it.
4. The return controller has no coverage of its own - the 405, the 403 on a bad reply and the cross-order reference check all live in it, but the kernel tests call onReturn() directly. They work (I checked live), so it's regression protection rather than a defect - a small request-level or functional test would lock it in.

No blockers from me. I'd recommend it for coverage.

Thanks Brian. Had a good follow up look - all four fixes are now present on 1.0.x at cafe3cb, re-verified on the same install as my original review. The denial branches now don't write anything - I re-fired the unsigned, forged and junk POSTs and the commerce_log no longer logs, while the signed flows still log as they should. The requirements deprecation is gone from the test run (the ones still showing belong to address, commerce and commerce_price) and the gateway discovers fine under the attribute. The new request-level tests take the suite to 43 tests / 690 assertions with every denial path asserting no payments and no log entities, which closes the controller gap. phpcs against your ruleset and phpstan level 6 both come back clean, and the full set of adversarial probes from my first pass still behaves at the new HEAD.

The fraction-digits change reads correctly at both call sites (the signing amount and the display string), though it's the one fix without a test of its own - a JPY or KWD case in the new kernel class would be good if you're in there again - but not a blocker by any means.

That's everything I raised, so marking it RTBC.

$this->fileSystem->realpath(self::CREDENTIALS_URI);
How do you justify this in light of FileSytemInterface::realpath() usage instructions Only use this function if you know that the stream wrapper in the URI uses the local file system ?

   // Credentials are read from a FIXED private file
    // (private://keys/cybersource.yml) and are deliberately NOT configurable
    // here — a configurable path would let an admin point the gateway at
    // attacker-controlled credentials. 

What was the security analysis on how a configurable path is less exploitable than a well known non-configurable path in private:// ? What factors did you include in your analysis?

Setting needs-work for answer to the above questions related to proper use of the Drupal API and secure code development.

I wonder what a configurable path would let an admin point the gateway at attacker-controlled credentials is supposed to mean or suggest.

I use Assisted AI but I review what it generates

I have concerns with that in the answer of comment 24.

I asked what the individual person used to evaluate, and the response to me appears typical of an LLM that has been caught with incorrect data scrambling to correct itself.

If that is the case the responses tells us little about the individuals knowledge and only informs us of the LLM's generated data. Prompt logs might be revealing if they indicate what the developer feed into the AI to direct the security design of those portions.

I chose the two points points very intentional.
The realpath() as while its a common error it is, to me(as a developer who works in the fringes impacted by realpath()), indicative of how well a coder is evaluating the LLM code as written against the Drupal Core API in full (and not just some method signature or copied from other modules).
The static file path as it touches on design choices and how they interact not just in a localized scope (inside the module) by including a global scope (considering a full Drupal install including all ancillary components). I am seeing some evaluation of the local scope though not sure I'm seeing a global scope understanding of the impact.

As @cmlara said, these applications aim to understand what the applicants understand about writing Drupal code which correctly uses the Drupal API, follows the Drupal coding standards, and does not contain possible security issues. Indirectly, we also check what they understand when asked to make changes, in preparation for security issues created for projects they maintain.
That is not possible, when changes to the project files, or responses to review comments, are done using AI tools.

Since you said I designed (from the beginning) that the configuration of keys should not be able to be modified by someone who has admin access to the website. may you list all the possibilities you considered as alternatives to using a static file path and the reason for which you discarded each solution?

Gaining access to an account with the right permissions would allow to change the values entered in a settings form. I would not use a form element that allows to directly enter the private key value, but rather a form element that allows to upload the file containing the private key; the person who gained access to the account would not be able to read the current private key value but it would still be possible to change the private key.
Effectively, a setting form would not be the right approach in this case.

Files uploaded/stored under private:// are still downloadable. Drupal core invokes a specific hook and, basing on what returned by the implementations of that hook, allows the file to be downloaded. In this case, using a hard-coded filename does not help.

I would rather do as Drupal core does for settings that cannot be changed using a setting form or implement the hook Drupal core uses to allow downloading a file which is not stored under public://.
In the first case, the filename would not be hardcoded; in the second case, knowing the filename would not help, except in the case the attacker finds a way to bypass the Drupal core protection for private:// files.

To expand on the previous posts, here is a list of weaknesses that I see of storing a key under private:// that come to mind on a cursory evaluation (more likely exist if one does a full risk analysis) (some of this might be a duplicate of previously made points):

• The path may be under the docroot of the Webserver(Pantehon hosting IIRC as one of the largest to do this) and all the risks that entails.
• The path is readable or writable by any user with private:// access (less likely to occur than a /raw/disk/path, the IMCE module being a well known example of this)
• The path is static known (easy for a simple remote arbitrary file read/write vulnerability to be exploited against)
• A path inside a streamWrapper does not have to escape a streamWrapper to gain raw disk access (If a code execution path prohibits /raw/disk/usage however allows scheme:// the code is a risk vector)
• There have been path access issues in the past where public:// could access private ( Partly fixed in SA-CORE-2022-012 for core, though contrib has also separately vulnerable such as SA-CONTRIB-2022-057 ) and must be presumed to occur again in the future.
• The path may be stored on a 2nd server expanding the security zone for key safety, even if that is just because private:// is an NFS share in the local network(Pantheon IIRC uses NFS for the public/private storage) (This doesn't even begin to consider when a site uses a module like s3fs or Flysystem to move private:// to a global server)
• The static path contains at least two keywords for a key stealer to identify and prioritize

Most of these do not appear by the statements to have been considered. Some of these require an additional weakness to be present while some are part of a normal Drupal site operation.

Note: If its an asymmetric key with Cybersource having the only private half the read threats may be somewhat minor.

Its unclear from the updated post if the applicant considered non UI methods of configuring the path (that would still require disk access or RCE of higher access than a simple private:// rewrite). The settings.php and Symonfy/Drupal Container both exist, the settings.php being a fundamental basic of Drupal development and site operation. It does appear that the developer excluded all raw disk as form of 'private storage'.

I am changing priority as per Issue priorities.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that does not follow the coding standards, contains possible security issue, or does not correctly use the Drupal API
• The single review points are not ordered, not even by importance

src/Controller/CybersourceReturnController.php

Since that class does not use methods from the parent class, or it uses a single method from the parent class, it does not need to use ControllerBase as parent class.

Controllers do not need to have a parent class; as long as they implement \Drupal\Core\DependencyInjection\ContainerInjectionInterface, they are fine.

With Drupal 10.2.x and higher versions, a controller class does not need to implement create(); using AutowireTrait, it just needs to implement the class constructor.

  public function __construct(
    protected CartSessionInterface $cartSession,
    protected LoggerInterface $logger,
  ) {}

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container): static {
    return new static(
      $container->get('commerce_cart.cart_session'),
      $container->get('logger.channel.cybersource_sop'),
    );
  }

Using a class property for a logger channel object is considered an anti-pattern. The class should have a property for the logger factory.

      $this->logger->warning('Cybersource return authentication failed for order @o: @m', [
        '@o' => $commerce_order->id(),
        '@m' => $e->getMessage(),
      ]);

The placeholders used in translatable messages are supposed to help translator for what the placeholder exactly is. It is better to use a longer placeholder name, which would give to the translators more information about the context in which the string is used.

src/EventSubscriber/CredentialsCheckSubscriber.php

Since the class is final, and cannot be extended, its properties can be private instead of protected.

  public function __construct(
    protected AdminContext $adminContext,
    protected AccountInterface $currentUser,
    protected EntityTypeManagerInterface $entityTypeManager,
    protected CredentialProvider $credentials,
    protected MessengerInterface $messenger,
    protected CredentialsStatus $credentialsStatus,
  ) {}

The current_user service implements \Drupal\Core\Session\AccountProxyInterface, which extends \Drupal\Core\Session\AccountInterface.

src/Hook/CybersourceSopHooks.php

/**
 * Object-oriented hook implementations for Cybersource SOP.
 *
 * Discovered automatically on Drupal 11.1+ via the #[Hook] attribute
 * (https://www.drupal.org/node/3442349). cybersource_sop.module and
 * cybersource_sop.install keep legacy stubs that delegate here (or to the
 * same services) so Drupal 10.3 still works; on modern cores the stubs are
 * skipped so each hook runs exactly once.
 *
 * Note for Drupal 10 compatibility: the #[Hook] attribute and the
 * RequirementSeverity enum do not exist there, but neither is resolved when
 * this class is merely loaded — attributes are only instantiated via
 * reflection (11.1+), and the enum is only referenced inside
 * runtimeRequirements(), which core only invokes on 11.3+.
 */

The long description in that documentation comment is not necessary. It is just a superfluous part added by AI.

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container): static {
    return new static(
      $container->get('cybersource_sop.credentials_status'),
    );

OOP classes do not need to implement that method, nor to implement ContainerInjectionInterface. They are service Drupal 11 automatically handle and like normal services, they do not need to implement create().

I did not verify what previously reported in a review and whenever that was completely fixed.

As a side note, this application was created using the brian.willows@hsj.co.uk account. That is the account which will get the permission to opt projects into security advisory coverage.

Since the hook class is set as service class using the following lines, there should no need to implement create(), like with any other service class.

  # Registered explicitly, keyed by class name, so the class resolver can build
  # it on Drupal 10.3, where #[Hook] classes are not auto-registered. Drupal
  # 11.1+ only auto-registers a hook class when the container has no definition
  # for it, so this definition is used on every supported core.
  Drupal\cybersource_sop\Hook\CybersourceSopHooks:
    class: Drupal\cybersource_sop\Hook\CybersourceSopHooks
    arguments: ['@cybersource_sop.credentials_status']

Thank you for your contribution and for your patience with the review process!

I am going to update your account so you can opt into security advisory coverage any project you create, including the projects you already created.

These are some recommended readings to help you with maintainership:

• Dries' post on Responsible maintainers
• Maintainership
• Git version control system
• Issue procedures and etiquette
• Maintaining and responding to issues for a project
• Release naming conventions.

You can find more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank also all the reviewers for helping with these applications.