`SmartCardConnect::startTransaction` API shape questions and considerations

[cmfcmf]

The SmartCardConnect::startTransaction API currently works like this:

type SmartCardDisposition =
  "leave" | "reset" | "unpower" | "eject";

const options = { signal: new AbortSignal() };

conn.startTransaction(async (): SmartCardDisposition | undefined => {
  await doSomeSmartCardStuff();
  return "reset";
}, options);

This makes me wonder about the following things:

Rejecting the promise always leads to a disposition of "leave"

If the promise passed to startTransaction is rejected, then, as per the specification, we always end the transaction with a disposition of "leave". If I understand the API and Smart Cards correctly, this would mean that every caller of startTransaction would likely always have to use a try {} finally {} block inside their callback if they want to make sure that it returns with their custom disposition:

// Bad:
conn.startTransaction(async () => {
  await doSomeSmartCardStuff(); // e.g., unlock the card
  await doMoreSmartCardStuff();
  return "reset"; // Bad: Never reached if any of the above code throws; will default to "leave".
});
// Good:
conn.startTransaction(async () => {
  try {
    await doSomeSmartCardStuff(); // e.g., unlock the card
    await doMoreSmartCardStuff();
  } finally {
    return "reset"; // Good: Always reached - and the card is always reset and no longer unlocked.
  }
});

You could of course argue that correctly using try {} finally {} is the developer's job, but I am wondering whether it would make sense to help the developer avoid this potential footgun? I know that "Provide an API that is at a higher level than PC/SC." is a non-goal - but given that startTransaction automatically calling endTransaction is a bit of an ever-so-slightly higher level API, I started wondering whether we could make it even so slightly more higher level.

Rough idea: Add another mandatory argument that is a callback that is run when the Promise is rejected.

conn.startTransaction(async (): SmartCardDisposition | undefined => {
  await doSomeSmartCardStuff(); // e.g., unlock the card
  await doMoreSmartCardStuff();
  return "reset";
}, (error): SmartCardDisposition | undefined => { // called only if the callback throws and the Promise is rejected
  return "reset";
});

Or make the callback argument optional, but let it handle both fulfilled and rejected promises:

conn.startTransaction(async (): void => {
  await doSomeSmartCardStuff(); // e.g., unlock the card
  await doMoreSmartCardStuff();
  // no return value here
}, (result, error): SmartCardDisposition | undefined => {
  // result is populated if the promise above resolves, error is populated otherwise
  return "reset";
});

How does the AbortSignal work?

Split off into #31.

[reillyeon]

To the first issue, we could add an option which controls the default disposition, with a default value of 'leave'. Passing multiple callbacks feels like re-implementing pieces of the existing Promise interface. As you say, using try {} finally {} properly to capture more complex error conditions is the developer's job.

[dandrader]

The web api eliminates the "forgetting to call endtransaction" problem, but creates a new one which is ending the transaction with a disposition different from what the main code path does when the code throws early and does not handle it properly. But this problem is inherent of programming languages with exception semantics.

I also think the proposals seem to reinvent existing mechanisms. Adding something like a defaultDisposition optional argument to the options dict is worth considering. But it still feels like reinventing try{}finally{} and further complicates the method's behavior. So I'm a bit reluctant.

[dandrader]

I would like to hear @domenic's opinion on this.

[domenic]

I tend to agree with @reillyeon and @dandrader's comments above. Especially about the proposals to add new callback APIs; those are almost exactly just reinventing promise.then().

The thing I don't understand (as someone who's not a domain expert) is why startTransaction() will reset to "leave" if the promise is rejected in the first place. I assume that the function passed to startTransaction() represents code that runs within a transaction. What is the natural behavior if someone has a logic error, or I/O error, or similar inside their transaction? Is it "reset" or "leave"? The current design implies that it's "leave". But it sounds like there's some complaint that this is "always" incorrect and instead should be "reset"? In that case, just change the API to default to "reset" when the promise is rejected.

[dandrader]

I assume that the function passed to startTransaction() represents code that runs within a transaction.

Correct.

What is the natural behavior if someone has a logic error, or I/O error, or similar inside their transaction? Is it "reset" or "leave"? The current design implies that it's "leave". But it sounds like there's some complaint that this is "always" incorrect and instead should be "reset"? In that case, just change the API to default to "reset" when the promise is rejected.

If the given transaction function has an implementation bug and failed to return the desired card disposition when ending the transaction, there's really no always-right default decision to take for that app. "leave" was chosen as it's the NOOP option, which is usually adequate for a default behavior.

But what makes this situation (transaction function ending without explicitly returning its desired card disposition) tricky is the security aspect involved in this. Poorly programmed PC/SC applications might unlock sensitive features of a card (such as private key operations) by requesting and applying a PIN and then, once done, leave the card as is, in that unlocked state. Malicious PC/SC applications lurking in the system can then exploit this bug to perform private key operations without the user knowledge, given that the card is already unlocked.

Microsoft's implementation of PC/SC, for instance, automatically ends a transaction if the application did not send any commands in the last five seconds (again, mitigating a buggy PC/SC app behavior). The interesting thing here is that it resets the card, thus cleaning up any state in it.

Thus I think it makes sense for this Web API to follow that mitigation approach and also end the transaction with a "reset" if the application fails to tell what it wanted to do. Changing the default disposition, as suggested by @domenic. It's also great as it saves us from expanding the API surface.

[cmfcmf]

I agree that defaulting to "reset" is probably best and mitigates my security concerns, thanks for the detailed comments!